diff --git a/core/workflows/review_pr.py b/core/workflows/review_pr.py index 45a3e30..2cdb7dc 100644 --- a/core/workflows/review_pr.py +++ b/core/workflows/review_pr.py @@ -86,6 +86,7 @@ _READY_TO_SPEC_LABEL = "ready-to-spec" _READY_TO_IMPLEMENT_LABEL = "ready-to-implement" _READY_LABELS = frozenset({_READY_TO_SPEC_LABEL, _READY_TO_IMPLEMENT_LABEL}) +_RESERVED_INTERNAL_LABEL = "warp:reserved-internal" # Allowed values for the agent-supplied ``verdict`` field on ``review.json``. _VERDICT_APPROVE = "APPROVE" @@ -176,6 +177,7 @@ class IssueReadinessStatus: labels: list[str] readiness_labels: list[str] has_required_label: bool + is_reserved_internal: bool is_pull_request: bool @@ -201,6 +203,7 @@ def _issue_readiness_status( labels=labels, readiness_labels=readiness_labels, has_required_label=required_label in labels and not is_pull_request, + is_reserved_internal=_RESERVED_INTERNAL_LABEL in labels and not is_pull_request, is_pull_request=is_pull_request, ) @@ -246,14 +249,22 @@ def check_pr_issue_state_for_review( for status in issue_statuses if status.has_required_label ] + reserved_internal_issue_numbers = [ + status.number + for status in issue_statuses + if status.is_reserved_internal + ] return { - "allowed": bool(ready_issue_numbers), + # Note: in practice, we don't expect triagers to assign both `ready-to-spec/implement` and `warp:reserved-internal` together. + # But if they do, we block the PR from being reviewed until `warp:reserved-internal` is removed. + "allowed": bool(ready_issue_numbers) and not bool(reserved_internal_issue_numbers), "spec_only": spec_only, "required_label": required_label, "association": association, "issue_numbers": issue_numbers, "issue_statuses": issue_statuses, "ready_issue_numbers": ready_issue_numbers, + "reserved_internal_issue_numbers": reserved_internal_issue_numbers, } @@ -284,14 +295,30 @@ def _format_pr_issue_state_failure_message( ) -> str: required_label = str(check["required_label"]) issue_numbers = [int(number) for number in check.get("issue_numbers") or []] + reserved_internal_issue_numbers = [ + int(number) for number in check.get("reserved_internal_issue_numbers") or [] + ] sections: list[str] = [] normalized_requester = requester.strip().removeprefix("@") if normalized_requester: sections.append(f"@{normalized_requester}") - sections.append( - "Every PR must be linked to a same-repo issue before Oz can review it." - ) - if issue_numbers: + if reserved_internal_issue_numbers: + issue_text = _format_issue_numbers(reserved_internal_issue_numbers) + sections.append( + f"This PR is linked to {issue_text}, which is marked " + f"`{_RESERVED_INTERNAL_LABEL}`. That label means the Warp team is " + "reserving the work for internal implementation or alignment, so " + "we are not accepting contributor PRs for it right now." + ) + sections.append( + "**Next step:** please look for another issue marked " + "`ready-to-spec` or `ready-to-implement`. If you think this label " + "was applied by mistake, mention `@oss-maintainers` on the issue." + ) + elif issue_numbers: + sections.append( + "Every PR must be linked to a same-repo issue before Oz can review it." + ) issue_text = _format_issue_numbers(issue_numbers) sections.append( f"This PR is linked to {issue_text}, but no linked issue is marked " @@ -300,6 +327,9 @@ def _format_pr_issue_state_failure_message( "push a new commit or comment `/oz-review` to re-trigger review." ) else: + sections.append( + "Every PR must be linked to a same-repo issue before Oz can review it." + ) sections.append( "**Next step:** open or find a same-repo issue describing this change, " "then link it to this PR by adding `Closes #123` to the PR description " diff --git a/tests/test_pr_issue_state_enforcement.py b/tests/test_pr_issue_state_enforcement.py index 0681180..1bfc11b 100644 --- a/tests/test_pr_issue_state_enforcement.py +++ b/tests/test_pr_issue_state_enforcement.py @@ -143,6 +143,18 @@ def test_wrong_label_fails_with_issue_status(self) -> None: self.assertFalse(check["allowed"]) self.assertEqual(check["issue_statuses"][0].readiness_labels, ["ready-to-spec"]) + def test_reserved_internal_label_overrides_ready_label(self) -> None: + check = self._check( + changed_files=["core/routing.py"], + issue_numbers=[18], + labels_by_issue={18: ["ready-to-implement", "warp:reserved-internal"]}, + github_linked_issue_numbers=[18], + ) + + self.assertFalse(check["allowed"]) + self.assertEqual(check["ready_issue_numbers"], [18]) + self.assertEqual(check["reserved_internal_issue_numbers"], [18]) + def test_multiple_issues_pass_when_any_issue_is_ready(self) -> None: check = self._check( changed_files=["core/routing.py"], @@ -297,6 +309,68 @@ def _create_review(body: str, event: str) -> None: ) self.assertEqual(reviews_created[0]["event"], "REQUEST_CHANGES") + def test_blocked_pr_with_reserved_internal_issue_rejects_contributor_pr(self) -> None: + pr_issue = _IssueWithComments() + linked_issue = _issue("ready-to-implement", "warp:reserved-internal") + repo = _Repo({10: pr_issue, 101: linked_issue}) + repo.default_branch = "main" + reviews_created: list[dict] = [] + + def _create_review(body: str, event: str) -> None: + reviews_created.append({"body": body, "event": event}) + + pr = SimpleNamespace( + number=10, + body="", + user=SimpleNamespace(login="external-user", type="User"), + author_association="NONE", + get_files=lambda: [_file("core/routing.py")], + create_review=_create_review, + ) + + association = { + "same_repo_issue_numbers": [101], + "github_linked_issues": [ + { + "owner": "acme", + "repo": "widgets", + "number": 101, + "source": "closingIssuesReferences", + } + ], + "deterministic_issue_numbers": [], + "primary_issue_number": 101, + "ambiguous": False, + } + with patch( + "workflows.review_pr.resolve_pr_association", + return_value=association, + ), patch( + "workflows.review_pr.decode_repo_text_file", + return_value="# Contributing", + ): + allowed = enforce_pr_issue_state_for_review( + repo, # type: ignore[arg-type] + owner="acme", + repo="widgets", + pr=pr, + requester="alice", + ) + + self.assertFalse(allowed) + self.assertEqual(len(pr_issue.comments), 1) + body = pr_issue.comments[0].body + self.assertIn("#101", body) + self.assertIn("`warp:reserved-internal`", body) + self.assertIn("reserving the work for internal implementation", body) + self.assertIn("not accepting contributor PRs", body) + self.assertIn( + "https://github.com/acme/widgets/blob/main/CONTRIBUTING.md", + body, + ) + self.assertEqual(reviews_created[0]["event"], "REQUEST_CHANGES") + self.assertIn("`warp:reserved-internal`", reviews_created[0]["body"]) + def test_blocked_pr_without_contributing_md_omits_link(self) -> None: pr_issue = _IssueWithComments() repo = _Repo({11: pr_issue}) diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 4341891..320c3d1 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -31,6 +31,7 @@ def test_create_implementation_prompt_uses_bare_shared_skill_names(self) -> None issue_labels=[], issue_assignees=[], spec_context_text="Spec body", + triggering_comment_text="", target_branch="oz-agent/implement-issue-42", default_branch="main", implement_specs_skill_path="warpdotdev/common-skills:.agents/skills/implement-specs/SKILL.md",