sysadmin
4dee03b2aa
Reviewer found the MCP merge surface is gated but two local CLI scripts remain ungated merge paths that LLM automations in this project have been using. Close them (Option B — minimal safe fix; full gated CLI merge left to a follow-up): - review_pr.py: `--merge` now fails closed BEFORE any API call with a clear message directing callers to the gated `gitea_merge_pr` MCP workflow. The review-only path is unchanged. The merge execution block was removed. - merge_pr.py: main() is now a fail-closed no-op — reads no credentials and makes no merge API call; prints that merge is only available via the gated workflow. - README: the `review_pr.py` row and Quick Examples no longer advertise a CLI `--merge` path; added an audit-logging clarification that #16 returns structured gate/merge results but does not add durable audit logging, which is tracked by #18. Tests updated/added: - test_review_pr.py: `--merge` fails closed with no API call; message points to the gated workflow. - test_merge_pr.py: merge fails closed with no API call, even with --force/--do/--title/--message; message points to the gated workflow. - test_mcp_server.py: README no longer advertises the ungated CLI merge example. The gated MCP `gitea_merge_pr` is unchanged and still gated; `gitea_review_pr` still fails closed on merge=True; `gitea_submit_pr_review` still cannot merge. No secrets, auth headers, raw env, or credential paths are exposed. No Jenkins/Ops/GlitchTip/Release/deploy/CI behavior added. #17/#18 not started. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
100 lines
3.4 KiB
Python
100 lines
3.4 KiB
Python
"""Tests for review_pr.py.
|
|
|
|
Mocks api_request and credentials.
|
|
"""
|
|
import sys
|
|
import unittest
|
|
from unittest.mock import patch
|
|
|
|
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
|
import review_pr # noqa: E402
|
|
|
|
|
|
FAKE_CREDS = "Basic dGVzdHVzZXI6dGVzdHBhc3M="
|
|
FAKE_PR_DATA = {
|
|
"number": 81,
|
|
"state": "open",
|
|
"head": {
|
|
"ref": "feature-branch",
|
|
"sha": "abcdef1234567890"
|
|
},
|
|
"base": {
|
|
"ref": "main"
|
|
},
|
|
"html_url": "https://gitea.example.com/pulls/81"
|
|
}
|
|
|
|
|
|
class TestArgParsing(unittest.TestCase):
|
|
|
|
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
|
def test_missing_pr_number_exits(self, _auth):
|
|
with self.assertRaises(SystemExit):
|
|
review_pr.main([])
|
|
|
|
|
|
class TestAPIPayload(unittest.TestCase):
|
|
|
|
@patch("review_pr.api_request")
|
|
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
|
def test_payload_fields_and_workflow(self, _auth, mock_api):
|
|
# Setup mock api_request to return PR details, then review response
|
|
mock_api.side_effect = [FAKE_PR_DATA, {}]
|
|
|
|
rc = review_pr.main([
|
|
"--pr-number", "81",
|
|
"--event", "APPROVE",
|
|
"--body", "Approved and ready to merge",
|
|
])
|
|
self.assertEqual(rc, 0)
|
|
self.assertEqual(mock_api.call_count, 2)
|
|
|
|
# Verify first call: GET PR
|
|
first_call_args = mock_api.call_args_list[0]
|
|
self.assertEqual(first_call_args[0][0], "GET")
|
|
self.assertEqual(first_call_args[0][1], "https://gitea.dadeschools.net/api/v1/repos/Contractor/Timesheet/pulls/81")
|
|
|
|
# Verify second call: POST review
|
|
second_call_args = mock_api.call_args_list[1]
|
|
self.assertEqual(second_call_args[0][0], "POST")
|
|
self.assertEqual(second_call_args[0][1], "https://gitea.dadeschools.net/api/v1/repos/Contractor/Timesheet/pulls/81/reviews")
|
|
payload = second_call_args[0][3]
|
|
self.assertEqual(payload["event"], "APPROVE")
|
|
self.assertEqual(payload["body"], "Approved and ready to merge")
|
|
self.assertEqual(payload["commit_id"], "abcdef1234567890")
|
|
|
|
@patch("review_pr.api_request")
|
|
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
|
def test_merge_flag_fails_closed_without_api_call(self, _auth, mock_api):
|
|
# --merge is an ungated bypass and is disabled (#16). It must fail
|
|
# closed BEFORE any API call — no review, no merge.
|
|
rc = review_pr.main([
|
|
"--pr-number", "81",
|
|
"--event", "APPROVE",
|
|
"--body", "Approved",
|
|
"--merge",
|
|
"--merge-method", "squash",
|
|
])
|
|
self.assertEqual(rc, 2)
|
|
self.assertEqual(mock_api.call_count, 0)
|
|
|
|
def test_merge_flag_message_points_to_gated_workflow(self):
|
|
import io
|
|
import contextlib
|
|
with patch("review_pr.get_auth_header", return_value=FAKE_CREDS), \
|
|
patch("review_pr.api_request") as mock_api:
|
|
buf = io.StringIO()
|
|
with contextlib.redirect_stderr(buf):
|
|
rc = review_pr.main([
|
|
"--pr-number", "81", "--event", "APPROVE", "--merge",
|
|
])
|
|
self.assertEqual(rc, 2)
|
|
self.assertEqual(mock_api.call_count, 0)
|
|
msg = buf.getvalue().lower()
|
|
self.assertIn("disabled", msg)
|
|
self.assertIn("gitea_merge_pr", msg)
|
|
|
|
|
|
if __name__ == "__main__":
|
|
unittest.main()
|