fix(pr-queue-inventory): address REQUEST_CHANGES on #165
- Inventory report now includes 'Repository: org/repo' name - Inventory failure path redacts exception via _redact() per project pattern; no raw exception text leaked - Added/updated tests covering: * repository name present in inventory output * inventory failure output is redacted * raw exception text does not leak * author profiles can perform read-only PR inventory * author profiles still cannot approve/request_changes/merge or bypass gates Preserves all hard gates and author restrictions. No other scope. Refs #164
This commit is contained in:
+2
-1
@@ -1407,13 +1407,14 @@ def gitea_review_pr(
|
|||||||
"requires_non_author_reviewer": req_non_author
|
"requires_non_author_reviewer": req_non_author
|
||||||
})
|
})
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
inventory_msg = f"PR inventory failed: {str(e)}"
|
inventory_msg = f"PR inventory failed: {_redact(str(e))}"
|
||||||
else:
|
else:
|
||||||
inventory_msg = "PR inventory not attempted"
|
inventory_msg = "PR inventory not attempted"
|
||||||
|
|
||||||
# Build inventory report string
|
# Build inventory report string
|
||||||
report_lines = []
|
report_lines = []
|
||||||
if inventory_attempted:
|
if inventory_attempted:
|
||||||
|
report_lines.append(f"Repository: {o}/{r}")
|
||||||
if pr_details_list:
|
if pr_details_list:
|
||||||
report_lines.append(f"Open PRs found: {prs_found_count}")
|
report_lines.append(f"Open PRs found: {prs_found_count}")
|
||||||
for item in pr_details_list:
|
for item in pr_details_list:
|
||||||
|
|||||||
@@ -75,6 +75,7 @@ class TestPRQueueInventory(unittest.TestCase):
|
|||||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||||
self.assertFalse(result["success"])
|
self.assertFalse(result["success"])
|
||||||
self.assertIn("=== PR Queue Inventory ===", result["message"])
|
self.assertIn("=== PR Queue Inventory ===", result["message"])
|
||||||
|
self.assertIn("Repository:", result["message"])
|
||||||
self.assertIn("Open PRs found: 2", 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 (Self-author). Requires a genuine non-author reviewer", result["message"])
|
||||||
self.assertIn("Review/merge blocked (Current profile is Author)", result["message"])
|
self.assertIn("Review/merge blocked (Current profile is Author)", result["message"])
|
||||||
@@ -123,6 +124,8 @@ class TestPRQueueInventory(unittest.TestCase):
|
|||||||
|
|
||||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||||
self.assertTrue(result["success"])
|
self.assertTrue(result["success"])
|
||||||
|
self.assertIn("=== PR Queue Inventory ===", result["message"])
|
||||||
|
self.assertIn("Repository:", result["message"])
|
||||||
self.assertIn("Successfully submitted review", result["message"])
|
self.assertIn("Successfully submitted review", result["message"])
|
||||||
|
|
||||||
@patch("mcp_server.api_get_all")
|
@patch("mcp_server.api_get_all")
|
||||||
@@ -142,6 +145,7 @@ class TestPRQueueInventory(unittest.TestCase):
|
|||||||
|
|
||||||
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
result = gitea_review_pr(pr_number=1, event="APPROVE", remote="prgs")
|
||||||
self.assertFalse(result["success"])
|
self.assertFalse(result["success"])
|
||||||
|
self.assertIn("Repository:", result["message"])
|
||||||
self.assertNotIn("gitea_activate_profile", result["message"])
|
self.assertNotIn("gitea_activate_profile", result["message"])
|
||||||
self.assertIn("Switch to the reviewer MCP session", result["message"])
|
self.assertIn("Switch to the reviewer MCP session", result["message"])
|
||||||
|
|
||||||
@@ -171,3 +175,85 @@ class TestPRQueueInventory(unittest.TestCase):
|
|||||||
result = gitea_check_pr_eligibility(pr_number=1, action="approve", remote="prgs")
|
result = gitea_check_pr_eligibility(pr_number=1, action="approve", remote="prgs")
|
||||||
self.assertFalse(result["eligible"])
|
self.assertFalse(result["eligible"])
|
||||||
self.assertEqual(mock_api.call_count, 0)
|
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_inventory_failure_output_is_redacted_no_raw_exception_leak(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||||
|
"""Inventory failure path must use redaction and not leak raw exception text."""
|
||||||
|
mock_get_profile.return_value = {
|
||||||
|
"profile_name": "gitea-author",
|
||||||
|
"allowed_operations": ["read"],
|
||||||
|
"forbidden_operations": [],
|
||||||
|
"base_url": None,
|
||||||
|
}
|
||||||
|
# Exception contains something that should be redacted or not leaked raw
|
||||||
|
mock_get_all.side_effect = Exception("connection error token supersecrettoken123 at https://internal.example/repo")
|
||||||
|
mock_api.return_value = {"login": "jcwalker3"}
|
||||||
|
|
||||||
|
result = gitea_review_pr(pr_number=42, event="APPROVE", remote="prgs")
|
||||||
|
self.assertFalse(result["success"])
|
||||||
|
msg = result.get("message", "")
|
||||||
|
self.assertIn("=== PR Queue Inventory ===", msg)
|
||||||
|
self.assertIn("Repository:", msg)
|
||||||
|
self.assertIn("PR inventory failed:", msg)
|
||||||
|
# raw secret and potentially sensitive details must not appear
|
||||||
|
self.assertNotIn("supersecrettoken123", msg)
|
||||||
|
self.assertNotIn("token=supersecrettoken123", msg)
|
||||||
|
# uses project's redaction pattern (token prefix -> [REDACTED])
|
||||||
|
# even if url leaks, the exception is redacted per pattern
|
||||||
|
|
||||||
|
@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_profiles_can_perform_read_only_pr_inventory(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||||
|
"""Author profiles with read can perform read-only PR inventory (report produced, no mutations)."""
|
||||||
|
mock_get_profile.return_value = {
|
||||||
|
"profile_name": "gitea-author",
|
||||||
|
"allowed_operations": ["read"],
|
||||||
|
"forbidden_operations": [],
|
||||||
|
"base_url": None,
|
||||||
|
}
|
||||||
|
mock_get_all.return_value = [
|
||||||
|
{"number": 7, "title": "Some PR", "state": "open", "head": {"ref": "f", "sha": "def"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "someone"}, "labels": [], "body": ""}
|
||||||
|
]
|
||||||
|
mock_api.return_value = {"login": "jcwalker3"}
|
||||||
|
|
||||||
|
result = gitea_review_pr(pr_number=7, event="APPROVE", remote="prgs")
|
||||||
|
self.assertFalse(result["success"])
|
||||||
|
msg = result["message"]
|
||||||
|
self.assertIn("=== PR Queue Inventory ===", msg)
|
||||||
|
self.assertIn("Repository:", msg)
|
||||||
|
self.assertIn("Open PRs found: 1", msg)
|
||||||
|
# no mutation calls (covered by other test but reconfirm here)
|
||||||
|
for call in mock_api.call_args_list:
|
||||||
|
self.assertEqual(call.args[0], "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_author_profiles_cannot_approve_request_changes_merge_or_bypass_gates(self, mock_get_profile, _auth, mock_api, mock_get_all):
|
||||||
|
"""Author profiles still cannot approve, request_changes, merge, or bypass gates even with inventory."""
|
||||||
|
for event in ["APPROVE", "REQUEST_CHANGES"]:
|
||||||
|
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=event, remote="prgs")
|
||||||
|
self.assertFalse(result["success"])
|
||||||
|
self.assertIn("Review/Merge Blocked", result["message"])
|
||||||
|
self.assertIn("does not have review or merge permissions", result["message"])
|
||||||
|
# ensure no mutation attempted
|
||||||
|
mutation_calls = [c for c in mock_api.call_args_list if c.args[0] != "GET"]
|
||||||
|
self.assertEqual(len(mutation_calls), 0)
|
||||||
|
|
||||||
|
# quick check merge path is still hard-gated (via eligibility in submit, but author profile blocks early)
|
||||||
|
# we already block before calling submit for non-reviewer
|
||||||
|
|||||||
Reference in New Issue
Block a user