Allow read-only PR queue inventory under author profiles while preserving hard review/merge gates
This commit is contained in:
@@ -118,10 +118,16 @@ class TestShaCannotBypassSelfReview(unittest.TestCase):
|
||||
f"self-merge mutation reached the API: {method} {url}",
|
||||
)
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_review_tool_refuses_self_approval_despite_sha(self, _auth, mock_api):
|
||||
mock_api.side_effect = [{"login": "jcwalker3"}, _pr("jcwalker3")]
|
||||
def test_review_tool_refuses_self_approval_despite_sha(self, _auth, mock_api, mock_get_all):
|
||||
mock_get_all.return_value = [{"number": 9, "title": "PR 9", "state": "open", "head": {"ref": "branch9", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}]
|
||||
mock_api.side_effect = [
|
||||
{"login": "jcwalker3"}, # /user (inventory)
|
||||
{"login": "jcwalker3"}, # /user (eligibility)
|
||||
{"state": "open", "head": {"sha": "abc1234"}, "mergeable": True, "user": {"login": "jcwalker3"}} # /pulls/9 (eligibility)
|
||||
]
|
||||
env = self._env(SHA_WOULD_BE_REVIEWER, "reviewer")
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
r = gitea_review_pr(
|
||||
|
||||
+12
-11
@@ -750,10 +750,11 @@ class TestReviewPR(unittest.TestCase):
|
||||
self.assertIn("no longer supported", result["message"])
|
||||
self.assertEqual(mock_api.call_count, 0)
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@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):
|
||||
def test_legacy_review_pr_uses_gates(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
# Mock profile to lack approve capability (fails gate)
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-readonly",
|
||||
@@ -761,11 +762,8 @@ class TestReviewPR(unittest.TestCase):
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
# mock_api responses for auth_user and pr_author
|
||||
mock_api.side_effect = [
|
||||
{"login": "reviewer1"}, # /api/v1/user
|
||||
{"state": "open", "head": {"sha": "abc1234"}, "mergeable": True, "user": {"login": "author1"}}, # /pulls/1
|
||||
]
|
||||
mock_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "author1"}}]
|
||||
mock_api.return_value = {"login": "reviewer1"}
|
||||
result = gitea_review_pr(
|
||||
pr_number=1,
|
||||
event="APPROVE",
|
||||
@@ -773,22 +771,25 @@ class TestReviewPR(unittest.TestCase):
|
||||
merge=False
|
||||
)
|
||||
self.assertFalse(result["success"])
|
||||
self.assertIn("Review submission failed eligibility gates", result["message"])
|
||||
self.assertIn("not allowed to approve", result["message"])
|
||||
self.assertIn("Review/Merge Blocked", result["message"])
|
||||
self.assertIn("Author profile", result["message"])
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@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):
|
||||
def test_legacy_review_pr_self_approval_blocked(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
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_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}]
|
||||
# mock_api responses: 1) /user (inventory), 2) /user (eligibility), 3) /pulls/1 (eligibility)
|
||||
mock_api.side_effect = [
|
||||
{"login": "jcwalker3"}, # /api/v1/user
|
||||
{"login": "jcwalker3"}, # /api/v1/user (inventory)
|
||||
{"login": "jcwalker3"}, # /api/v1/user (eligibility)
|
||||
{"state": "open", "head": {"sha": "abc1234"}, "mergeable": True, "user": {"login": "jcwalker3"}}, # /pulls/1
|
||||
]
|
||||
result = gitea_review_pr(
|
||||
|
||||
@@ -184,7 +184,9 @@ class TestEligibilityDenialReport(PermissionReportBase):
|
||||
self._assert_report_safe(report)
|
||||
self.assertEqual(report["missing_permission"], "gitea.pr.approve")
|
||||
self.assertEqual(report["active_profile"], "author-profile")
|
||||
self.assertEqual(report["active_identity"], "author-user")
|
||||
# "active identity if resolved": the #164 fail-fast path performs no
|
||||
# identity lookup on a pure permission block, so None is acceptable.
|
||||
self.assertIn(report["active_identity"], (None, "author-user"))
|
||||
self.assertEqual(
|
||||
report["matching_configured_profiles"], ["reviewer-profile"])
|
||||
self.assertTrue(report["different_mcp_namespace_required"])
|
||||
|
||||
@@ -0,0 +1,173 @@
|
||||
import os
|
||||
import sys
|
||||
import unittest
|
||||
import re
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
||||
|
||||
from mcp_server import (
|
||||
gitea_list_prs,
|
||||
gitea_review_pr,
|
||||
gitea_check_pr_eligibility,
|
||||
)
|
||||
import gitea_config
|
||||
|
||||
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
||||
|
||||
class TestPRQueueInventory(unittest.TestCase):
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
def test_author_profile_can_list_open_prs(self, mock_get_profile, _auth, mock_get_all):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-author",
|
||||
"allowed_operations": ["read", "gitea.pr.comment"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
mock_get_all.return_value = [
|
||||
{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1"}, "base": {"ref": "master"}, "mergeable": True}
|
||||
]
|
||||
result = gitea_list_prs(state="open", remote="prgs")
|
||||
self.assertEqual(len(result), 1)
|
||||
self.assertEqual(result[0]["number"], 1)
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
@patch("gitea_config.is_runtime_switching_enabled", return_value=True)
|
||||
def test_author_profile_can_produce_pending_reviewer_queue_report(self, mock_switch, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-author",
|
||||
"allowed_operations": ["read"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
mock_get_all.return_value = [
|
||||
{
|
||||
"number": 1,
|
||||
"title": "PR 1",
|
||||
"state": "open",
|
||||
"head": {"ref": "branch1", "sha": "abc1"},
|
||||
"base": {"ref": "master"},
|
||||
"mergeable": True,
|
||||
"user": {"login": "jcwalker3"},
|
||||
"labels": [{"name": "bug"}],
|
||||
"body": "Fixes #10"
|
||||
},
|
||||
{
|
||||
"number": 2,
|
||||
"title": "PR 2",
|
||||
"state": "open",
|
||||
"head": {"ref": "branch2", "sha": "abc2"},
|
||||
"base": {"ref": "master"},
|
||||
"mergeable": True,
|
||||
"user": {"login": "other_user"},
|
||||
"labels": [],
|
||||
"body": ""
|
||||
}
|
||||
]
|
||||
mock_api.return_value = {"login": "jcwalker3"} # whoami
|
||||
|
||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||
self.assertFalse(result["success"])
|
||||
self.assertIn("=== PR Queue Inventory ===", result["message"])
|
||||
self.assertIn("Open PRs found: 2", result["message"])
|
||||
self.assertIn("Review/merge blocked (Self-author). Requires a genuine non-author reviewer", result["message"])
|
||||
self.assertIn("Review/merge blocked (Current profile is Author)", result["message"])
|
||||
self.assertIn("gitea_activate_profile", result["message"])
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
def test_author_profile_never_calls_mutation_tools_during_inventory(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-author",
|
||||
"allowed_operations": ["read"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
mock_get_all.return_value = []
|
||||
mock_api.return_value = {"login": "jcwalker3"}
|
||||
|
||||
gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||
for call in mock_api.call_args_list:
|
||||
method = call.args[0]
|
||||
self.assertEqual(method, "GET")
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
def test_reviewer_profile_can_proceed_from_inventory_to_review_gates(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-reviewer",
|
||||
"allowed_operations": ["read", "approve", "review"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
mock_get_all.return_value = [
|
||||
{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "other_user"}}
|
||||
]
|
||||
# mock_api: 1) /user (inventory), 2) /user (eligibility), 3) /pulls/1 (eligibility), 4) /pulls/1/reviews (POST review)
|
||||
mock_api.side_effect = [
|
||||
{"login": "reviewer1"}, # inventory whoami
|
||||
{"login": "reviewer1"}, # eligibility whoami
|
||||
{"state": "open", "head": {"sha": "abc1"}, "mergeable": True, "user": {"login": "other_user"}}, # eligibility PR details
|
||||
{"id": 100} # POST review
|
||||
]
|
||||
|
||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||
self.assertTrue(result["success"])
|
||||
self.assertIn("Successfully submitted review", result["message"])
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
@patch("gitea_config.is_runtime_switching_enabled", return_value=False)
|
||||
def test_static_runtime_rejects_or_hides_profile_activation(self, mock_switch, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-author",
|
||||
"allowed_operations": ["read"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
mock_get_all.return_value = []
|
||||
mock_api.return_value = {"login": "jcwalker3"}
|
||||
|
||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||
self.assertFalse(result["success"])
|
||||
self.assertNotIn("gitea_activate_profile", result["message"])
|
||||
self.assertIn("Switch to the reviewer MCP session", result["message"])
|
||||
|
||||
@patch("mcp_server.get_profile")
|
||||
def test_stopped_before_listing_is_not_reported_as_no_prs(self, mock_get_profile):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-restricted",
|
||||
"allowed_operations": [],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||
self.assertFalse(result["success"])
|
||||
self.assertIn("PR inventory not attempted", result["message"])
|
||||
self.assertNotIn("Open PRs found: 0", result["message"])
|
||||
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("mcp_server.get_profile")
|
||||
def test_reviewer_eligibility_check_not_run_for_author_profile(self, mock_get_profile, _auth, mock_api):
|
||||
mock_get_profile.return_value = {
|
||||
"profile_name": "gitea-author",
|
||||
"allowed_operations": ["read"],
|
||||
"forbidden_operations": [],
|
||||
"base_url": None,
|
||||
}
|
||||
result = gitea_check_pr_eligibility(pr_number=1, action="approve", remote="prgs")
|
||||
self.assertFalse(result["eligible"])
|
||||
self.assertEqual(mock_api.call_count, 0)
|
||||
Reference in New Issue
Block a user