diff --git a/README.md b/README.md index 352a07c..fb12133 100644 --- a/README.md +++ b/README.md @@ -180,6 +180,9 @@ Notes: can inspect which runtime it is talking to before deciding to act. - See [`docs/gitea-execution-profiles.md`](docs/gitea-execution-profiles.md) for the full profile model. +- **Audit logging:** #16 returns structured gate/merge results but does not add + durable audit logging. Durable audit logging for Gitea MCP mutating actions is + tracked by #18.
@@ -204,7 +207,7 @@ The MCP tools can also be used as standalone CLI scripts: | `create_issue.py` | Create an issue (`--remote`, `--title`, `--body`, `--body-file`) | | `create_pr.py` | Open a Pull Request (`--remote`, `--title`, `--head`, `--base`) | | `edit_pr.py` | Edit a Pull Request (`--title`, `--body`, `--body-file`, etc.) | -| `review_pr.py` | Review and sign-off on a pull request (with optional merge) | +| `review_pr.py` | Review/sign-off on a pull request (`--merge` is disabled — fails closed; merge only via gated `gitea_merge_pr`) | | `close_issue.py` | Close a specific issue | | `mark_issue.py` | Claim/release an issue via `status:in-progress` label | | `manage_labels.py` | Create label set and apply label mappings (`--dry` to preview) | @@ -225,8 +228,9 @@ The MCP tools can also be used as standalone CLI scripts: # Edit a PR's description or title ./edit_pr.py 155 --body "Updated description wording" -# Review and approve a PR, then automatically merge it -./review_pr.py --pr-number 12 --event APPROVE --body "Approved" --merge +# Review and approve a PR (review only — CLI merge is disabled; use the +# gated gitea_merge_pr MCP workflow to merge) +./review_pr.py --pr-number 12 --event APPROVE --body "Approved" # Close issue #5 ./close_issue.py 5 diff --git a/merge_pr.py b/merge_pr.py index c34c8d4..6f5ded6 100755 --- a/merge_pr.py +++ b/merge_pr.py @@ -1,7 +1,13 @@ #!/usr/bin/env python3 -"""Merge a Gitea pull request. +"""Merge a Gitea pull request — DISABLED (#16). -Usage: +Direct CLI merge is a fail-closed no-op. LLM automations in this project were +using this script as an ungated merge bypass, so it no longer performs a merge. +Merge is only available through the gated `gitea_merge_pr` MCP workflow (#16), +which enforces identity/profile/eligibility, explicit confirmation, expected +head SHA checking, and self-merge protection. + +Usage (fails closed): merge_pr.py --pr-number 84 --do squash --remote dadeschools """ import os @@ -29,29 +35,20 @@ def main(argv=None): parser.add_argument("--force", action="store_true", help="Force merge, ignoring status checks.") args = parser.parse_args(argv) - host, org, repo = resolve_remote(args) - auth = get_auth_header(host) - if not auth: - print(f"Could not get credentials for {host}.", file=sys.stderr) - return 1 - - url = f"{repo_api_url(host, org, repo)}/pulls/{args.pr_number}/merge" - payload = { - "Do": args.do, - "force_merge": args.force, - } - if args.title: - payload["MergeTitleField"] = args.title - if args.message: - payload["MergeMessageField"] = args.message - - try: - api_request("POST", url, auth, payload) - print(f"Successfully merged PR #{args.pr_number} using '{args.do}' method.") - return 0 - except Exception as e: - print(f"Error merging PR #{args.pr_number}: {e}", file=sys.stderr) - return 1 + # Fail closed: direct CLI merge is disabled (#16). No credentials are read + # and no merge API call is made. Merge is only available through the gated + # `gitea_merge_pr` MCP workflow, which enforces identity/profile/eligibility, + # explicit confirmation, expected head SHA checking, and self-merge + # protection. + print( + f"Direct CLI merge is disabled (#16). PR #{args.pr_number} was NOT " + "merged. Merge is only available through the gated workflow (MCP tool " + "'gitea_merge_pr'), which enforces identity/profile/eligibility, " + "explicit confirmation, expected head SHA checking, and self-merge " + "protection.", + file=sys.stderr, + ) + return 2 if __name__ == "__main__": diff --git a/review_pr.py b/review_pr.py index 374b4e8..3a1ceaf 100755 --- a/review_pr.py +++ b/review_pr.py @@ -37,6 +37,22 @@ def main(argv=None): help="Merge method/style to use if merging (default: merge).") args = parser.parse_args(argv) + # Fail closed: direct CLI merge is disabled (#16). LLM automations were + # using this flag as an ungated merge bypass. Merge is only available via + # the gated `gitea_merge_pr` MCP workflow, which enforces + # identity/profile/eligibility, explicit confirmation, expected head SHA, + # and self-merge protection. No API call is made here. + if args.merge: + print( + "Direct CLI merge is disabled. Merge is only available through the " + "gated #16 workflow (MCP tool 'gitea_merge_pr'), which enforces " + "identity/profile/eligibility, explicit confirmation, expected head " + "SHA checking, and self-merge protection. Re-run without --merge to " + "submit a review only.", + file=sys.stderr, + ) + return 2 + host, org, repo = resolve_remote(args) body = args.body @@ -80,25 +96,8 @@ def main(argv=None): print(f"Error submitting review: {e}", file=sys.stderr) return 1 - # 3. Merge PR if --merge is requested and event is APPROVE - if args.merge: - if args.event != "APPROVE": - print("Warning: Skipping merge because review event is not 'APPROVE'.", file=sys.stderr) - return 0 - - merge_url = f"{repo_api_url(host, org, repo)}/pulls/{args.pr_number}/merge" - merge_payload = { - "Do": args.merge_method, - "force_merge": False - } - - try: - api_request("POST", merge_url, auth, merge_payload) - print(f"Successfully merged PR #{args.pr_number} using '{args.merge_method}' method.") - except Exception as e: - print(f"Error merging PR: {e}", file=sys.stderr) - return 1 - + # Merge is intentionally not performed here — see the fail-closed guard + # above. Use the gated `gitea_merge_pr` MCP workflow (#16) to merge. return 0 diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index c7440c8..b62c2d3 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -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 diff --git a/tests/test_merge_pr.py b/tests/test_merge_pr.py index 66a7ca9..4dca610 100644 --- a/tests/test_merge_pr.py +++ b/tests/test_merge_pr.py @@ -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__": diff --git a/tests/test_review_pr.py b/tests/test_review_pr.py index 0d0ac7f..119bb95 100644 --- a/tests/test_review_pr.py +++ b/tests/test_review_pr.py @@ -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__":