diff --git a/README.md b/README.md index d8c93e0..c124ee8 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ Any MCP-compatible agent (Antigravity, Claude Code, etc.) can call these tools n | `gitea_list_prs` | List pull requests with state/remote | | `gitea_view_pr` | Get full details of a single pull request | | `gitea_merge_pr` | Merge a pull request (merge, squash, or rebase) | -| `gitea_review_pr` | Submit a review on a pull request and optionally merge it | +| `gitea_review_pr` | Legacy wrapper for `gitea_submit_pr_review` (merging disabled) | | `gitea_delete_branch` | Delete a remote branch | | `gitea_close_issue` | Close an issue by number | | `gitea_list_issues` | List issues with state/label filters | diff --git a/mcp_server.py b/mcp_server.py index 17154f2..6ba1a81 100644 --- a/mcp_server.py +++ b/mcp_server.py @@ -726,14 +726,18 @@ def gitea_review_pr( org: str | None = None, repo: str | None = None, ) -> dict: - """Submit a review on a Gitea pull request and optionally merge it. + """Submit a review on a Gitea pull request (Legacy wrapper). + + This tool is a compatibility wrapper around the safe `gitea_submit_pr_review`. + It uses the same #14 eligibility gates. + Merging via this tool is no longer supported and will fail closed (see #16). Args: pr_number: The PR number to review. event: Review type — 'APPROVE', 'COMMENT', or 'REQUEST_CHANGES'. body: Review body text / comment. - merge: If True and event is 'APPROVE', automatically merge the PR. - merge_method: Merge style to use if merging — 'merge', 'squash', or 'rebase'. + merge: Merging is disabled; if True, the tool fails closed. + merge_method: Ignored. remote: Known instance — 'dadeschools' or 'prgs'. host: Override the Gitea host. org: Override the owner/organization. @@ -742,45 +746,39 @@ def gitea_review_pr( Returns: dict with success status and message. """ + if merge: + return { + "success": False, + "message": "merge=True is no longer supported in this tool (belongs to #16)." + } + if event not in ["APPROVE", "COMMENT", "REQUEST_CHANGES"]: raise ValueError(f"Invalid review event: '{event}'. Choose from 'APPROVE', 'COMMENT', 'REQUEST_CHANGES'.") - if merge_method not in ["merge", "squash", "rebase"]: - raise ValueError(f"Invalid merge method: '{merge_method}'. Choose from 'merge', 'squash', 'rebase'.") - h, o, r = _resolve(remote, host, org, repo) - auth = _auth(h) - - # 1. Fetch PR to get the latest head commit SHA (required for review payload) - pr_url = f"{repo_api_url(h, o, r)}/pulls/{pr_number}" - pr_data = api_request("GET", pr_url, auth) - commit_sha = pr_data.get("head", {}).get("sha") - if not commit_sha: - raise RuntimeError(f"Could not find head commit SHA for PR #{pr_number}.") - - # 2. Submit the PR review - review_url = f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews" - payload = { - "body": body, - "event": event, - "commit_id": commit_sha + # Map legacy event string to the action expected by gitea_submit_pr_review + event_map = { + "APPROVE": "approve", + "COMMENT": "comment", + "REQUEST_CHANGES": "request_changes" } - api_request("POST", review_url, auth, payload) - msg = f"Successfully submitted review for PR #{pr_number} with event '{event}'." + action = event_map[event] - # 3. Merge PR if merge is True and event is APPROVE - if merge: - if event != "APPROVE": - msg += " Warning: Skipping merge because review event is not 'APPROVE'." - else: - merge_url = f"{repo_api_url(h, o, r)}/pulls/{pr_number}/merge" - merge_payload = { - "Do": merge_method, - "force_merge": False - } - api_request("POST", merge_url, auth, merge_payload) - msg += f" Successfully merged PR #{pr_number} using '{merge_method}' method." + result = gitea_submit_pr_review( + pr_number=pr_number, + action=action, + body=body, + expected_head_sha=None, + remote=remote, + host=host, + org=org, + repo=repo + ) - return {"success": True, "message": msg} + if result.get("performed"): + return {"success": True, "message": f"Successfully submitted review for PR #{pr_number} with event '{event}'."} + else: + reasons = result.get("reasons", []) + return {"success": False, "message": f"Review submission failed eligibility gates: {reasons}"} @mcp.tool() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 993e3b6..3f86942 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -321,38 +321,67 @@ class TestReviewPR(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_review_pr_and_merge(self, _auth, mock_api): - # GET PR response (fetch head SHA) + def test_legacy_review_pr_merge_fails_closed(self, _auth, mock_api): + result = gitea_review_pr( + pr_number=1, + event="APPROVE", + body="Looks good", + merge=True + ) + self.assertFalse(result["success"]) + self.assertIn("no longer supported", result["message"]) + self.assertEqual(mock_api.call_count, 0) + + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + @patch("mcp_server.get_profile") + def test_legacy_review_pr_uses_gates(self, mock_get_profile, _auth, mock_api): + # Mock profile to lack approve capability (fails gate) + mock_get_profile.return_value = { + "profile_name": "gitea-readonly", + "allowed_operations": ["read"], + "forbidden_operations": [], + "base_url": None, + } + # mock_api responses for auth_user and pr_author mock_api.side_effect = [ - {"head": {"sha": "sha-val-123"}}, # GET PR pulls/1 - {}, # POST review - {}, # POST merge + {"login": "reviewer1"}, # /api/v1/user + {"state": "open", "head": {"sha": "abc1234"}, "mergeable": True, "user": {"login": "author1"}}, # /pulls/1 ] result = gitea_review_pr( pr_number=1, event="APPROVE", body="Looks good", - merge=True, - merge_method="squash" + merge=False ) - self.assertTrue(result["success"]) - self.assertIn("Successfully submitted review", result["message"]) - self.assertIn("Successfully merged", result["message"]) - - # Check call counts and arguments - self.assertEqual(mock_api.call_count, 3) - - # Verify GET PR - self.assertEqual(mock_api.call_args_list[0][0][0], "GET") - - # Verify POST review - self.assertEqual(mock_api.call_args_list[1][0][0], "POST") - self.assertEqual(mock_api.call_args_list[1][0][3]["event"], "APPROVE") - self.assertEqual(mock_api.call_args_list[1][0][3]["commit_id"], "sha-val-123") - - # Verify POST merge - self.assertEqual(mock_api.call_args_list[2][0][0], "POST") - self.assertEqual(mock_api.call_args_list[2][0][3]["Do"], "squash") + self.assertFalse(result["success"]) + self.assertIn("Review submission failed eligibility gates", result["message"]) + self.assertIn("not allowed to approve", result["message"]) + + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + @patch("mcp_server.get_profile") + def test_legacy_review_pr_self_approval_blocked(self, mock_get_profile, _auth, mock_api): + mock_get_profile.return_value = { + "profile_name": "gitea-reviewer", + "allowed_operations": ["read", "approve"], + "forbidden_operations": [], + "base_url": None, + } + # mock_api responses for auth_user and pr_author + mock_api.side_effect = [ + {"login": "jcwalker3"}, # /api/v1/user + {"state": "open", "head": {"sha": "abc1234"}, "mergeable": True, "user": {"login": "jcwalker3"}}, # /pulls/1 + ] + result = gitea_review_pr( + pr_number=1, + event="APPROVE", + body="Self approve", + merge=False + ) + self.assertFalse(result["success"]) + self.assertIn("Review submission failed eligibility gates", result["message"]) + self.assertIn("authenticated user is PR author", result["message"]) # ---------------------------------------------------------------------------