fix: close ungated CLI merge bypasses in review_pr.py and merge_pr.py (#16)
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>
This commit is contained in:
@@ -602,6 +602,17 @@ class TestNoUngatedMergePath(unittest.TestCase):
|
||||
merge_src = inspect.getsource(mcp_server.gitea_merge_pr)
|
||||
self.assertIn("/merge\"", merge_src)
|
||||
|
||||
def test_readme_no_longer_advertises_ungated_cli_merge(self):
|
||||
import pathlib
|
||||
readme = (pathlib.Path(__file__).resolve().parent.parent
|
||||
/ "README.md").read_text(encoding="utf-8")
|
||||
# The old ungated example command must be gone.
|
||||
self.assertNotIn('--body "Approved" --merge', readme)
|
||||
# And the gated messaging / audit deferral must be present.
|
||||
self.assertIn("disabled", readme.lower())
|
||||
self.assertIn("gitea_merge_pr", readme)
|
||||
self.assertIn("#18", readme)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Review PR
|
||||
|
||||
+26
-27
@@ -15,49 +15,48 @@ FAKE_CREDS = "Basic dGVzdHVzZXI6dGVzdHBhc3M="
|
||||
|
||||
class TestArgParsing(unittest.TestCase):
|
||||
|
||||
@patch("merge_pr.api_request")
|
||||
@patch("merge_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_minimal_required_args(self, _auth, mock_api):
|
||||
rc = merge_pr.main(["--pr-number", "81"])
|
||||
self.assertEqual(rc, 0)
|
||||
mock_api.assert_called_once()
|
||||
|
||||
def test_missing_pr_number_exits(self):
|
||||
with self.assertRaises(SystemExit):
|
||||
merge_pr.main([])
|
||||
|
||||
|
||||
class TestAPIPayload(unittest.TestCase):
|
||||
class TestMergeDisabled(unittest.TestCase):
|
||||
"""Direct CLI merge is disabled (#16) — fails closed, no API call."""
|
||||
|
||||
@patch("merge_pr.api_request")
|
||||
@patch("merge_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_payload_fields(self, _auth, mock_api):
|
||||
def test_merge_fails_closed_without_api_call(self, _auth, mock_api):
|
||||
rc = merge_pr.main(["--pr-number", "81"])
|
||||
self.assertEqual(rc, 2)
|
||||
mock_api.assert_not_called()
|
||||
|
||||
@patch("merge_pr.api_request")
|
||||
@patch("merge_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_no_merge_even_with_force_and_method(self, _auth, mock_api):
|
||||
rc = merge_pr.main([
|
||||
"--pr-number", "81",
|
||||
"--do", "squash",
|
||||
"--title", "Squash title",
|
||||
"--message", "Squash message",
|
||||
"--force",
|
||||
"--remote", "prgs",
|
||||
])
|
||||
self.assertEqual(rc, 0)
|
||||
mock_api.assert_called_once()
|
||||
method, url, auth, payload = mock_api.call_args[0]
|
||||
self.assertEqual(method, "POST")
|
||||
self.assertEqual(auth, FAKE_CREDS)
|
||||
self.assertEqual(payload["Do"], "squash")
|
||||
self.assertEqual(payload["MergeTitleField"], "Squash title")
|
||||
self.assertEqual(payload["MergeMessageField"], "Squash message")
|
||||
self.assertEqual(payload["force_merge"], True)
|
||||
self.assertEqual(rc, 2)
|
||||
mock_api.assert_not_called()
|
||||
|
||||
@patch("merge_pr.api_request")
|
||||
@patch("merge_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_url_construction(self, _auth, mock_api):
|
||||
merge_pr.main(["--pr-number", "81", "--remote", "prgs"])
|
||||
url = mock_api.call_args[0][1]
|
||||
self.assertEqual(
|
||||
url,
|
||||
"https://gitea.prgs.cc/api/v1/repos/Scaled-Tech-Consulting/Timesheet/pulls/81/merge"
|
||||
)
|
||||
def test_message_points_to_gated_workflow(self):
|
||||
import io
|
||||
import contextlib
|
||||
with patch("merge_pr.get_auth_header", return_value=FAKE_CREDS), \
|
||||
patch("merge_pr.api_request") as mock_api:
|
||||
buf = io.StringIO()
|
||||
with contextlib.redirect_stderr(buf):
|
||||
rc = merge_pr.main(["--pr-number", "81"])
|
||||
self.assertEqual(rc, 2)
|
||||
mock_api.assert_not_called()
|
||||
msg = buf.getvalue().lower()
|
||||
self.assertIn("disabled", msg)
|
||||
self.assertIn("gitea_merge_pr", msg)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
+21
-14
@@ -65,27 +65,34 @@ class TestAPIPayload(unittest.TestCase):
|
||||
|
||||
@patch("review_pr.api_request")
|
||||
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_approve_and_merge_workflow(self, _auth, mock_api):
|
||||
# Setup mock api_request to return PR details, review response, and merge response
|
||||
mock_api.side_effect = [FAKE_PR_DATA, {}, {}]
|
||||
|
||||
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"
|
||||
"--merge-method", "squash",
|
||||
])
|
||||
self.assertEqual(rc, 0)
|
||||
self.assertEqual(mock_api.call_count, 3)
|
||||
self.assertEqual(rc, 2)
|
||||
self.assertEqual(mock_api.call_count, 0)
|
||||
|
||||
# Verify third call: POST merge
|
||||
third_call_args = mock_api.call_args_list[2]
|
||||
self.assertEqual(third_call_args[0][0], "POST")
|
||||
self.assertEqual(third_call_args[0][1], "https://gitea.dadeschools.net/api/v1/repos/Contractor/Timesheet/pulls/81/merge")
|
||||
payload = third_call_args[0][3]
|
||||
self.assertEqual(payload["Do"], "squash")
|
||||
self.assertEqual(payload["force_merge"], False)
|
||||
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__":
|
||||
|
||||
Reference in New Issue
Block a user