fix: close gitea_review_pr ungated bypass (#15)

2026-07-01 14:42:00 -04:00
parent f05e58c847
commit 6c992a4fa7
3 changed files with 89 additions and 62 deletions
@@ -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 |
@@ -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()
@@ -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"])
+        self.assertFalse(result["success"])
+        self.assertIn("Review submission failed eligibility gates", result["message"])
+        self.assertIn("not allowed to approve", 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")
+    @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"])


 # ---------------------------------------------------------------------------