diff --git a/Makefile b/Makefile index f1947b5..f6f047a 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ UV ?= uv UV_REQUIRED_VERSION := 0.10.11 -.PHONY: check-uv init lint fmt test i18n docker +.PHONY: check-uv init lint fmt test run start i18n docker check-uv: @command -v $(UV) >/dev/null 2>&1 || { echo "uv is required. Install uv $(UV_REQUIRED_VERSION) before continuing."; exit 1; } @@ -23,6 +23,11 @@ fmt: check-uv test: lint @$(UV) run pytest tests +run: check-uv + @$(UV) run python gitlab_bot.py + +start: run + i18n: xgettext -d base -o src/locales/gitlab-bot.pot *.py msgfmt -o src/locales/en/LC_MESSAGES/gitlab-bot.mo src/locales/en/LC_MESSAGES/gitlab-bot.po diff --git a/README.md b/README.md index 0c11340..9617ddc 100644 --- a/README.md +++ b/README.md @@ -69,9 +69,10 @@ Use the existing Makefile entry points for common tasks: make lint make fmt make test +make run ``` -These commands use `uv.lock`; they do not install packages globally. +`make run` starts the local webhook service using the configuration from `.env`. `make start` is an alias. These commands use `uv.lock`; they do not install packages globally. ## How to use @@ -221,6 +222,8 @@ Merge requests can only be merged if the commit message follows a specific forma This parameter controls whether the bot automatically approves merge requests that pass all checks. By default, it is set to `true`, allowing the bot to automatically approve merge requests. Setting it to `false` disables automatic approval, requiring manual approval for all merge requests. +When validation fails, the bot only attempts to revoke its own approval. It never resets other users' approvals; if the approval state cannot be confirmed, the failure comment reports that state instead of claiming the approval was revoked. + **`BOT_GITLAB_MERGE_REQUEST_AIREVIEW_LABEL_ENABLED`** This parameter controls the feature to add a new status label "AI Review" to merge requests, facilitating better tracking of AI-assisted code reviews. When enabled, the label is automatically added when AI summary generation is enabled and completed. The label is removed when a Merge Request is updated but the AI feature is disabled. diff --git a/gitlab_bot.py b/gitlab_bot.py index b59941c..d1f1dc6 100644 --- a/gitlab_bot.py +++ b/gitlab_bot.py @@ -13,9 +13,9 @@ # limitations under the License. import logging +import warnings from dotenv import load_dotenv -from gidgetlab.aiohttp import GitLabBot from src.config import ( bot_gitlab_token, @@ -31,6 +31,27 @@ load_dotenv() # isort:skip + +def _load_gitlab_bot(): + # gidgetlab 1.1.0 still imports the deprecated pkg_resources API. + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", + category=UserWarning, + message=r"pkg_resources is deprecated as an API\.", + ) + warnings.filterwarnings( + "ignore", + category=DeprecationWarning, + message=r"Deprecated call to `pkg_resources\.declare_namespace", + ) + from gidgetlab.aiohttp import GitLabBot + + return GitLabBot + + +GitLabBot = _load_gitlab_bot() + bot = GitLabBot(bot_gitlab_username, url=bot_gitlab_url, access_token=bot_gitlab_token) issue_hooks = IssueHooks() diff --git a/pyproject.toml b/pyproject.toml index 6e6e532..798e8eb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,10 +26,12 @@ authors = [ requires-python = ">=3.9,<4.0" dependencies = [ "gidgetlab[aiohttp]>=1.1.0,<2.0.0", + "setuptools<81", "langchain==0.3.6", "langchain-openai==0.2.5", "langchain-google-genai==2.0.4", "python-dotenv==1.0.1", + "urllib3<2", ] [dependency-groups] diff --git a/src/locales/en/LC_MESSAGES/gitlab-bot.mo b/src/locales/en/LC_MESSAGES/gitlab-bot.mo index 99c47c7..9ad5d62 100644 Binary files a/src/locales/en/LC_MESSAGES/gitlab-bot.mo and b/src/locales/en/LC_MESSAGES/gitlab-bot.mo differ diff --git a/src/locales/en/LC_MESSAGES/gitlab-bot.po b/src/locales/en/LC_MESSAGES/gitlab-bot.po index 7917211..4dde89a 100644 --- a/src/locales/en/LC_MESSAGES/gitlab-bot.po +++ b/src/locales/en/LC_MESSAGES/gitlab-bot.po @@ -33,7 +33,27 @@ msgid "bot_review_success" msgstr "😊 Review validation success and approve the merge request." msgid "bot_review_fails" -msgstr "🙁 Review validation fails and approval has been revoked." +msgstr "🙁 Review validation failed." +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_revoked" +msgstr "🙁 Review validation failed and the bot's approval was revoked." +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_not_present" +msgstr "🙁 Review validation failed; no bot approval was present, so no approval was revoked." +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_not_revoked" +msgstr "🙁 Review validation failed; the bot approval could not be revoked." +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_unknown" +msgstr "🙁 Review validation failed; the bot approval status could not be confirmed." "\n" "{error_message}" diff --git a/src/locales/zh/LC_MESSAGES/gitlab-bot.mo b/src/locales/zh/LC_MESSAGES/gitlab-bot.mo index 8d50dd5..0858152 100644 Binary files a/src/locales/zh/LC_MESSAGES/gitlab-bot.mo and b/src/locales/zh/LC_MESSAGES/gitlab-bot.mo differ diff --git a/src/locales/zh/LC_MESSAGES/gitlab-bot.po b/src/locales/zh/LC_MESSAGES/gitlab-bot.po index bdc9754..ec5eb29 100644 --- a/src/locales/zh/LC_MESSAGES/gitlab-bot.po +++ b/src/locales/zh/LC_MESSAGES/gitlab-bot.po @@ -33,7 +33,27 @@ msgid "bot_review_success" msgstr "😊合并请求验证成功，批准合并请求。" msgid "bot_review_fails" -msgstr "🙁合并请求验证失败，并撤回批准。" +msgstr "🙁合并请求验证失败。" +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_revoked" +msgstr "🙁合并请求验证失败，机器人审批已撤销。" +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_not_present" +msgstr "🙁合并请求验证失败；未发现机器人审批，因此没有撤销任何审批。" +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_not_revoked" +msgstr "🙁合并请求验证失败；机器人审批未能撤销。" +"\n" +"{error_message}" + +msgid "bot_review_fails_approval_unknown" +msgstr "🙁合并请求验证失败；无法确认机器人审批状态。" "\n" "{error_message}" @@ -72,4 +92,4 @@ msgstr "Bot 帮助: \n" "* **/bot-issue-outdated**: 添加 ~\"Outdated\" 标签到最近 17 天不活跃的 Issues，例如: /bot-issue-outdated" msgid "marked_issue_outdated_summary" -msgstr "发现 {total} 个过时的问题（闲置超过 {days_ago} 天），标记为 ~\"{label_name}\"" \ No newline at end of file +msgstr "发现 {total} 个过时的问题（闲置超过 {days_ago} 天），标记为 ~\"{label_name}\"" diff --git a/src/merge_request_hook.py b/src/merge_request_hook.py index 38f2a52..6f59f78 100644 --- a/src/merge_request_hook.py +++ b/src/merge_request_hook.py @@ -191,6 +191,7 @@ async def generate_diff_description_summary(event, gl): async def check_commit(event, gl): project_id = event.project_id + approval_attempted = False if event.data["event_type"] == "note": commit_title = event.data["merge_request"]["last_commit"]["title"] commit_author_name = event.data["merge_request"]["last_commit"]["author"]["name"] @@ -224,32 +225,54 @@ async def check_commit(event, gl): check_email(commit_author_name, commit_author_email) check_commit_message(commit_title) if bot_gitlab_merge_request_approval_enabled: + approval_attempted = True + await approval_merge_request(project_id, iid, gl) message = _("bot_review_success") - approval_merge_request(project_id, iid, gl) await gl.post(merge_request_post_note_url, data={"body": message}) except Exception as e: - message = _("bot_review_fails").format(error_message=str(e)) + if bot_gitlab_merge_request_approval_enabled and not approval_attempted: + try: + bot_approval_revoked = await unapprove_merge_request(project_id, iid, gl) + except Exception as unapprove_error: + message = _("bot_review_fails_approval_not_revoked").format( + error_message=f"{e}; unapprove error: {unapprove_error}" + ) + else: + if bot_approval_revoked: + message = _("bot_review_fails_approval_revoked").format(error_message=str(e)) + else: + message = _("bot_review_fails_approval_not_present").format(error_message=str(e)) + elif bot_gitlab_merge_request_approval_enabled: + message = _("bot_review_fails_approval_unknown").format(error_message=str(e)) + else: + message = _("bot_review_fails").format(error_message=str(e)) await gl.post(merge_request_post_note_url, data={"body": message}) - # Only support GitLab Premium in 13.9 - # https://docs.gitlab.com/ee/api/merge_request_approvals.html#unapprove-merge-request - # merge_request_post_unapproval_url = ( - # f"/projects/{project_id}/merge_requests/{iid}/unapprove" - # ) - # await gl.post(merge_request_post_unapproval_url, data=None) + +async def _bot_has_approval(project_id, iid, gl): + query_approvals_url = f"/projects/{project_id}/merge_requests/{iid}/approvals" + approvals = await gl.getitem(query_approvals_url) + if approvals.get("approved"): + for approval in approvals.get("approved_by") or []: + user = approval.get("user") or {} + if user.get("username") == bot_gitlab_username: + return True + return False async def approval_merge_request(project_id, iid, gl): - query_approvals_url = f"/projects/{project_id}/merge_requests/{iid}/approvals" - approvals = gl.getitem(query_approvals_url) - bot_approved = False - if approvals.approved: - for approval in approvals.approved_by: - if approval.user.username == bot_gitlab_username: - bot_approved = True - return - if not bot_approved: - await gl.post(f"/projects/{project_id}/merge_requests/{iid}/approve", data=None) + if await _bot_has_approval(project_id, iid, gl): + return + + await gl.post(f"/projects/{project_id}/merge_requests/{iid}/approve", data=None) + + +async def unapprove_merge_request(project_id, iid, gl): + if not await _bot_has_approval(project_id, iid, gl): + return False + + await gl.post(f"/projects/{project_id}/merge_requests/{iid}/unapprove", data=None) + return True def is_opened_merge_request(event): diff --git a/tests/fixtures/__init__.py b/tests/fixtures/__init__.py new file mode 100644 index 0000000..093ec55 --- /dev/null +++ b/tests/fixtures/__init__.py @@ -0,0 +1 @@ +"""Reusable fixtures for GitLab bot tests.""" diff --git a/tests/fixtures/approval_contract.py b/tests/fixtures/approval_contract.py new file mode 100644 index 0000000..545e434 --- /dev/null +++ b/tests/fixtures/approval_contract.py @@ -0,0 +1,81 @@ +from copy import deepcopy + +APPROVALS_EMPTY_GET = { + "approved": False, + "approvals_required": None, + "approvals_left": None, + "approved_by": [], +} + +APPROVALS_ROBOT_GET = { + "approved": True, + "approvals_required": None, + "approvals_left": None, + "approved_by": [ + { + "user": { + "id": 28, + "username": "review-bot", + "name": "review-bot", + } + } + ], +} + +APPROVALS_OTHER_USER_GET = { + "approved": True, + "approvals_required": None, + "approvals_left": None, + "approved_by": [ + { + "user": { + "id": 29, + "username": "other-reviewer", + "name": "Other Reviewer", + } + } + ], +} + +APPROVE_RESPONSE = { + "user_has_approved": True, + "user_can_approve": False, + "approved": True, + "approved_by": [ + { + "user": { + "id": 28, + "username": "review-bot", + "name": "review-bot", + } + } + ], +} + +UNAPPROVE_RESPONSE = { + "user_has_approved": False, + "user_can_approve": True, + "approved": False, + "approved_by": [], +} + + +class ApprovalApiFake: + """Async fake for the subset of gidgetlab used by approval tests.""" + + def __init__(self, responses): + self._responses = {key: list(values) for key, values in responses.items()} + self.calls = [] + + async def getitem(self, url): + return self._respond("GET", url, None) + + async def post(self, url, data=None): + return self._respond("POST", url, data) + + def _respond(self, method, url, data): + self.calls.append((method, url, data)) + key = (method, url) + if key not in self._responses or not self._responses[key]: + raise AssertionError(f"No fake response configured for {method} {url}") + return deepcopy(self._responses[key].pop(0)) diff --git a/tests/test_merge_request_approval_contract.py b/tests/test_merge_request_approval_contract.py new file mode 100644 index 0000000..ff4fd96 --- /dev/null +++ b/tests/test_merge_request_approval_contract.py @@ -0,0 +1,113 @@ +import asyncio + +import src.merge_request_hook as merge_request_hook +from tests.fixtures.approval_contract import ( + APPROVALS_EMPTY_GET, + APPROVALS_OTHER_USER_GET, + APPROVALS_ROBOT_GET, + APPROVE_RESPONSE, + UNAPPROVE_RESPONSE, + ApprovalApiFake, +) + +approval_merge_request = merge_request_hook.approval_merge_request + +APPROVALS_URL = "/projects/76/merge_requests/1/approvals" +APPROVE_URL = "/projects/76/merge_requests/1/approve" +UNAPPROVE_URL = "/projects/76/merge_requests/1/unapprove" + + +def test_target_approvals_fixture_preserves_nullable_fields(): + client = ApprovalApiFake({("GET", APPROVALS_URL): [APPROVALS_EMPTY_GET]}) + + response = asyncio.run(client.getitem(APPROVALS_URL)) + + assert response == { + "approved": False, + "approvals_required": None, + "approvals_left": None, + "approved_by": [], + } + + +def test_approval_fixtures_distinguish_robot_other_user_and_empty_state(): + assert APPROVALS_EMPTY_GET["approved_by"] == [] + assert APPROVALS_ROBOT_GET["approved_by"][0]["user"]["username"] == "review-bot" + assert APPROVALS_OTHER_USER_GET["approved_by"][0]["user"]["username"] == "other-reviewer" + + +def test_async_fake_records_approval_api_order_and_payload(): + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_EMPTY_GET], + ("POST", APPROVE_URL): [APPROVE_RESPONSE], + ("POST", UNAPPROVE_URL): [UNAPPROVE_RESPONSE], + } + ) + + async def exercise_api(): + await client.getitem(APPROVALS_URL) + await client.post(APPROVE_URL, data=None) + await client.post(UNAPPROVE_URL, data=None) + + asyncio.run(exercise_api()) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + ("POST", UNAPPROVE_URL, None), + ] + + +def test_approve_and_unapprove_fixtures_capture_target_user_state(): + assert APPROVE_RESPONSE["user_has_approved"] is True + assert APPROVE_RESPONSE["user_can_approve"] is False + assert APPROVE_RESPONSE["approved_by"][0]["user"]["username"] == "review-bot" + + assert UNAPPROVE_RESPONSE["user_has_approved"] is False + assert UNAPPROVE_RESPONSE["user_can_approve"] is True + assert UNAPPROVE_RESPONSE["approved"] is False + assert UNAPPROVE_RESPONSE["approved_by"] == [] + + +def test_approval_merge_request_approves_when_robot_is_not_approved(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_EMPTY_GET], + ("POST", APPROVE_URL): [APPROVE_RESPONSE], + } + ) + + asyncio.run(approval_merge_request(76, 1, client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + ] + + +def test_approval_merge_request_does_not_duplicate_robot_approval(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + client = ApprovalApiFake({("GET", APPROVALS_URL): [APPROVALS_ROBOT_GET]}) + + asyncio.run(approval_merge_request(76, 1, client)) + + assert client.calls == [("GET", APPROVALS_URL, None)] + + +def test_approval_merge_request_approves_when_only_other_user_is_approved(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_OTHER_USER_GET], + ("POST", APPROVE_URL): [APPROVE_RESPONSE], + } + ) + + asyncio.run(approval_merge_request(76, 1, client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + ] diff --git a/tests/test_merge_request_check_commit.py b/tests/test_merge_request_check_commit.py new file mode 100644 index 0000000..a83126e --- /dev/null +++ b/tests/test_merge_request_check_commit.py @@ -0,0 +1,99 @@ +import asyncio +from types import SimpleNamespace + +import src.merge_request_hook as merge_request_hook +from tests.fixtures.approval_contract import ( + APPROVALS_ROBOT_GET, + APPROVE_RESPONSE, + ApprovalApiFake, +) + +PROJECT_ID = 76 +MR_IID = 1 +COMMITS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/commits" +APPROVALS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approvals" +APPROVE_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approve" +NOTES_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/notes" + + +def make_merge_request_event(): + return SimpleNamespace( + project_id=PROJECT_ID, + data={ + "event_type": "merge_request", + "object_attributes": { + "last_commit": { + "title": "[feat]:[][Test commit]", + "author": { + "name": "reviewer", + "email": "reviewer@asiainfo.com", + }, + }, + "iid": MR_IID, + "milestone_id": 1, + "description": "#123 test merge request", + }, + }, + ) + + +def success_note_call(): + return ("POST", NOTES_URL, {"body": merge_request_hook._("bot_review_success")}) + + +def test_check_commit_awaits_approval_before_success_note(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", True) + client = ApprovalApiFake( + { + ("GET", COMMITS_URL): [[]], + ("GET", APPROVALS_URL): [ + { + "approved": False, + "approvals_required": None, + "approvals_left": None, + "approved_by": [], + } + ], + ("POST", APPROVE_URL): [APPROVE_RESPONSE], + ("POST", NOTES_URL): [{}], + } + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + success_note_call(), + ] + + +def test_check_commit_skips_approve_when_robot_already_approved(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", True) + client = ApprovalApiFake( + { + ("GET", COMMITS_URL): [[]], + ("GET", APPROVALS_URL): [APPROVALS_ROBOT_GET], + ("POST", NOTES_URL): [{}], + } + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + success_note_call(), + ] + + +def test_check_commit_does_not_call_approval_api_when_feature_is_disabled(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", False) + client = ApprovalApiFake({("GET", COMMITS_URL): [[]]}) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [("GET", COMMITS_URL, None)] diff --git a/tests/test_merge_request_failure_state.py b/tests/test_merge_request_failure_state.py new file mode 100644 index 0000000..0ae77e9 --- /dev/null +++ b/tests/test_merge_request_failure_state.py @@ -0,0 +1,188 @@ +import asyncio +from types import SimpleNamespace + +import src.merge_request_hook as merge_request_hook +from tests.fixtures.approval_contract import ( + APPROVALS_EMPTY_GET, + APPROVALS_OTHER_USER_GET, + APPROVALS_ROBOT_GET, + UNAPPROVE_RESPONSE, + ApprovalApiFake, +) + +PROJECT_ID = 76 +MR_IID = 1 +COMMITS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/commits" +APPROVALS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approvals" +APPROVE_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approve" +UNAPPROVE_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/unapprove" +NOTES_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/notes" + + +def make_merge_request_event(): + return SimpleNamespace( + project_id=PROJECT_ID, + data={ + "event_type": "merge_request", + "object_attributes": { + "last_commit": { + "title": "[feat]:[][Test commit]", + "author": { + "name": "reviewer", + "email": "reviewer@asiainfo.com", + }, + }, + "iid": MR_IID, + "milestone_id": 1, + "description": "#123 test merge request", + }, + }, + ) + + +class FailingGetApprovalApiFake(ApprovalApiFake): + def __init__(self, responses, failing_url, error): + super().__init__(responses) + self.failing_url = failing_url + self.error = error + + async def getitem(self, url): + if url == self.failing_url: + self.calls.append(("GET", url, None)) + raise self.error + return self._respond("GET", url, None) + + +class FailingPostApprovalApiFake(ApprovalApiFake): + def __init__(self, responses, failing_url, error): + super().__init__(responses) + self.failing_url = failing_url + self.error = error + + async def post(self, url, data=None): + if url == self.failing_url: + self.calls.append(("POST", url, data)) + raise self.error + return self._respond("POST", url, data) + + +def failure_note(key, error_message): + return ( + "POST", + NOTES_URL, + {"body": merge_request_hook._(key).format(error_message=error_message)}, + ) + + +def fail_validation(message="validation failed"): + def check_commit_message(_): + raise RuntimeError(message) + + return check_commit_message + + +def configure_failure_path(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", True) + monkeypatch.setattr(merge_request_hook, "check_commit_message", fail_validation()) + + +def test_check_commit_unapproves_only_robot_approval_on_validation_failure(monkeypatch): + configure_failure_path(monkeypatch) + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_ROBOT_GET], + ("POST", UNAPPROVE_URL): [UNAPPROVE_RESPONSE], + ("POST", NOTES_URL): [{}], + } + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", UNAPPROVE_URL, None), + failure_note("bot_review_fails_approval_revoked", "validation failed"), + ] + + +def test_check_commit_preserves_other_users_approval_on_validation_failure(monkeypatch): + configure_failure_path(monkeypatch) + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_OTHER_USER_GET], + ("POST", NOTES_URL): [{}], + } + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + failure_note("bot_review_fails_approval_not_present", "validation failed"), + ] + + +def test_check_commit_reports_unapprove_failure_without_claiming_revoked(monkeypatch): + configure_failure_path(monkeypatch) + client = FailingPostApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_ROBOT_GET], + ("POST", NOTES_URL): [{}], + }, + UNAPPROVE_URL, + RuntimeError("unapprove failed"), + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", UNAPPROVE_URL, None), + failure_note( + "bot_review_fails_approval_not_revoked", + "validation failed; unapprove error: unapprove failed", + ), + ] + + +def test_check_commit_reports_approval_query_failure_without_claiming_revoked(monkeypatch): + configure_failure_path(monkeypatch) + client = FailingGetApprovalApiFake( + {("POST", NOTES_URL): [{}]}, + APPROVALS_URL, + RuntimeError("approvals unavailable"), + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", APPROVALS_URL, None), + failure_note( + "bot_review_fails_approval_not_revoked", + "validation failed; unapprove error: approvals unavailable", + ), + ] + + +def test_check_commit_reports_approve_failure_without_attempting_unapprove(monkeypatch): + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", True) + client = FailingPostApprovalApiFake( + { + ("GET", COMMITS_URL): [[]], + ("GET", APPROVALS_URL): [APPROVALS_EMPTY_GET], + ("POST", NOTES_URL): [{}], + }, + APPROVE_URL, + RuntimeError("approve failed"), + ) + + asyncio.run(merge_request_hook.check_commit(make_merge_request_event(), client)) + + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + failure_note("bot_review_fails_approval_unknown", "approve failed"), + ] diff --git a/tests/test_merge_request_webhook_regression.py b/tests/test_merge_request_webhook_regression.py new file mode 100644 index 0000000..ccd8623 --- /dev/null +++ b/tests/test_merge_request_webhook_regression.py @@ -0,0 +1,231 @@ +import asyncio +from types import SimpleNamespace + +import pytest + +import src.merge_request_hook as merge_request_hook +from tests.fixtures.approval_contract import ( + APPROVALS_EMPTY_GET, + APPROVALS_ROBOT_GET, + APPROVE_RESPONSE, + UNAPPROVE_RESPONSE, + ApprovalApiFake, +) + +PROJECT_ID = 76 +MR_IID = 1 +COMMITS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/commits" +APPROVALS_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approvals" +APPROVE_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/approve" +UNAPPROVE_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/unapprove" +NOTES_URL = f"/projects/{PROJECT_ID}/merge_requests/{MR_IID}/notes" + + +def make_event(event_type="merge_request", state="opened"): + last_commit = { + "title": "[feat]:[][Test commit]", + "author": { + "name": "reviewer", + "email": "reviewer@asiainfo.com", + }, + } + if event_type == "note": + return SimpleNamespace( + project_id=PROJECT_ID, + data={ + "event_type": "note", + "object_attributes": {"note": "/bot-review"}, + "merge_request": { + "state": state, + "last_commit": last_commit, + "iid": MR_IID, + "milestone_id": 1, + "description": "#123 test merge request", + }, + }, + ) + + return SimpleNamespace( + project_id=PROJECT_ID, + data={ + "event_type": "merge_request", + "object_attributes": { + "state": state, + "last_commit": last_commit, + "iid": MR_IID, + "milestone_id": 1, + "description": "#123 test merge request", + }, + }, + ) + + +def success_note_call(): + return ("POST", NOTES_URL, {"body": merge_request_hook._("bot_review_success")}) + + +def failure_note_call(key, error_message): + return ( + "POST", + NOTES_URL, + {"body": merge_request_hook._(key).format(error_message=error_message)}, + ) + + +def make_success_client(approvals=APPROVALS_EMPTY_GET): + return ApprovalApiFake( + { + ("GET", COMMITS_URL): [[]], + ("GET", APPROVALS_URL): [approvals], + ("POST", APPROVE_URL): [APPROVE_RESPONSE], + ("POST", NOTES_URL): [{}], + } + ) + + +class FailingPostApiFake(ApprovalApiFake): + def __init__(self, responses, failing_url, error): + super().__init__(responses) + self.failing_url = failing_url + self.error = error + + async def post(self, url, data=None): + if url == self.failing_url: + self.calls.append(("POST", url, data)) + raise self.error + return self._respond("POST", url, data) + + +class FailingGetApiFake(ApprovalApiFake): + def __init__(self, responses, failing_url, error): + super().__init__(responses) + self.failing_url = failing_url + self.error = error + + async def getitem(self, url): + if url == self.failing_url: + self.calls.append(("GET", url, None)) + raise self.error + return self._respond("GET", url, None) + + +def configure_hooks(monkeypatch): + async def skip_summary(event, gl): + return None + + monkeypatch.setattr(merge_request_hook, "generate_diff_description_summary", skip_summary) + monkeypatch.setattr(merge_request_hook, "has_required_reviewer", lambda event_data: True) + monkeypatch.setattr(merge_request_hook, "bot_gitlab_username", "review-bot") + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", True) + + +@pytest.mark.parametrize( + "method_name,event_type", + [ + ("merge_request_opened_event", "merge_request"), + ("merge_request_updated_event", "merge_request"), + ("merge_request_reopen_event", "merge_request"), + ("note_merge_request_event", "note"), + ], +) +def test_merge_request_hooks_entrypoints_preserve_approval_order(monkeypatch, method_name, event_type): + configure_hooks(monkeypatch) + client = make_success_client() + hooks = merge_request_hook.MergeRequestHooks() + + result = asyncio.run(getattr(hooks, method_name)(make_event(event_type), client)) + + assert result is None + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + success_note_call(), + ] + + +def test_merge_request_hook_failure_returns_once_after_bot_unapproval(monkeypatch): + configure_hooks(monkeypatch) + + def fail_validation(_): + raise RuntimeError("validation failed") + + monkeypatch.setattr(merge_request_hook, "check_commit_message", fail_validation) + client = ApprovalApiFake( + { + ("GET", APPROVALS_URL): [APPROVALS_ROBOT_GET], + ("POST", UNAPPROVE_URL): [UNAPPROVE_RESPONSE], + ("POST", NOTES_URL): [{}], + } + ) + hooks = merge_request_hook.MergeRequestHooks() + + result = asyncio.run(hooks.merge_request_opened_event(make_event(), client)) + + assert result is None + assert client.calls == [ + ("GET", APPROVALS_URL, None), + ("POST", UNAPPROVE_URL, None), + failure_note_call("bot_review_fails_approval_revoked", "validation failed"), + ] + assert sum(call[0:2] == ("POST", NOTES_URL) for call in client.calls) == 1 + + +def test_merge_request_hook_approval_failure_returns_once_without_retry(monkeypatch): + configure_hooks(monkeypatch) + client = FailingPostApiFake( + { + ("GET", COMMITS_URL): [[]], + ("GET", APPROVALS_URL): [APPROVALS_EMPTY_GET], + ("POST", NOTES_URL): [{}], + }, + APPROVE_URL, + RuntimeError("approve failed"), + ) + hooks = merge_request_hook.MergeRequestHooks() + + result = asyncio.run(hooks.merge_request_updated_event(make_event(), client)) + + assert result is None + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + ("POST", APPROVE_URL, None), + failure_note_call("bot_review_fails_approval_unknown", "approve failed"), + ] + assert sum(call[0:2] == ("POST", NOTES_URL) for call in client.calls) == 1 + + +def test_merge_request_hook_approval_query_failure_returns_once_without_retry(monkeypatch): + configure_hooks(monkeypatch) + client = FailingGetApiFake( + { + ("GET", COMMITS_URL): [[]], + ("POST", NOTES_URL): [{}], + }, + APPROVALS_URL, + RuntimeError("approvals unavailable"), + ) + hooks = merge_request_hook.MergeRequestHooks() + + result = asyncio.run(hooks.merge_request_updated_event(make_event(), client)) + + assert result is None + assert client.calls == [ + ("GET", COMMITS_URL, None), + ("GET", APPROVALS_URL, None), + failure_note_call("bot_review_fails_approval_unknown", "approvals unavailable"), + ] + assert sum(call[0:2] == ("POST", NOTES_URL) for call in client.calls) == 1 + + +def test_merge_request_hook_flag_disabled_skips_approval_endpoints(monkeypatch): + configure_hooks(monkeypatch) + monkeypatch.setattr(merge_request_hook, "bot_gitlab_merge_request_approval_enabled", False) + client = ApprovalApiFake({("GET", COMMITS_URL): [[]]}) + hooks = merge_request_hook.MergeRequestHooks() + + result = asyncio.run(hooks.merge_request_updated_event(make_event(), client)) + + assert result is None + assert client.calls == [("GET", COMMITS_URL, None)] diff --git a/tests/test_runtime_startup.py b/tests/test_runtime_startup.py new file mode 100644 index 0000000..d09bc43 --- /dev/null +++ b/tests/test_runtime_startup.py @@ -0,0 +1,4 @@ +def test_gidgetlab_runtime_import_is_available(): + from gitlab_bot import GitLabBot + + assert GitLabBot is not None diff --git a/uv.lock b/uv.lock index 93a1e12..6f9cda6 100644 --- a/uv.lock +++ b/uv.lock @@ -451,6 +451,8 @@ dependencies = [ { name = "langchain-google-genai" }, { name = "langchain-openai" }, { name = "python-dotenv" }, + { name = "setuptools" }, + { name = "urllib3" }, ] [package.dev-dependencies] @@ -468,6 +470,8 @@ requires-dist = [ { name = "langchain-google-genai", specifier = "==2.0.4" }, { name = "langchain-openai", specifier = "==0.2.5" }, { name = "python-dotenv", specifier = "==1.0.1" }, + { name = "setuptools", specifier = "<81" }, + { name = "urllib3", specifier = "<2" }, ] [package.metadata.requires-dev] @@ -1720,6 +1724,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/aa/70/f8724f31abc0b329ca98b33d73c14020168babcf71b0cba3cded5d9d0e66/ruff-0.7.4-py3-none-win_arm64.whl", hash = "sha256:11bff065102c3ae9d3ea4dc9ecdfe5a5171349cdd0787c1fc64761212fc9cf1f", size = 8851590, upload-time = "2024-11-15T11:33:09.664Z" }, ] +[[package]] +name = "setuptools" +version = "80.10.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/76/95/faf61eb8363f26aa7e1d762267a8d602a1b26d4f3a1e758e92cb3cb8b054/setuptools-80.10.2.tar.gz", hash = "sha256:8b0e9d10c784bf7d262c4e5ec5d4ec94127ce206e8738f29a437945fbc219b70", size = 1200343, upload-time = "2026-01-25T22:38:17.252Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/94/b8/f1f62a5e3c0ad2ff1d189590bfa4c46b4f3b6e49cef6f26c6ee4e575394d/setuptools-80.10.2-py3-none-any.whl", hash = "sha256:95b30ddfb717250edb492926c92b5221f7ef3fbcc2b07579bcd4a27da21d0173", size = 1064234, upload-time = "2026-01-25T22:38:15.216Z" }, +] + [[package]] name = "sniffio" version = "1.3.1" @@ -1904,11 +1917,11 @@ wheels = [ [[package]] name = "urllib3" -version = "2.3.0" +version = "1.26.20" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/aa/63/e53da845320b757bf29ef6a9062f5c669fe997973f966045cb019c3f4b66/urllib3-2.3.0.tar.gz", hash = "sha256:f8c5449b3cf0861679ce7e0503c7b44b5ec981bec0d1d3795a07f1ba96f0204d", size = 307268, upload-time = "2024-12-22T07:47:30.032Z" } +sdist = { url = "https://files.pythonhosted.org/packages/e4/e8/6ff5e6bc22095cfc59b6ea711b687e2b7ed4bdb373f7eeec370a97d7392f/urllib3-1.26.20.tar.gz", hash = "sha256:40c2dc0c681e47eb8f90e7e27bf6ff7df2e677421fd46756da1161c39ca70d32", size = 307380, upload-time = "2024-08-29T15:43:11.37Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/c8/19/4ec628951a74043532ca2cf5d97b7b14863931476d117c471e8e2b1eb39f/urllib3-2.3.0-py3-none-any.whl", hash = "sha256:1cee9ad369867bfdbbb48b7dd50374c0967a0bb7710050facf0dd6911440e3df", size = 128369, upload-time = "2024-12-22T07:47:28.074Z" }, + { url = "https://files.pythonhosted.org/packages/33/cf/8435d5a7159e2a9c83a95896ed596f68cf798005fe107cc655b5c5c14704/urllib3-1.26.20-py2.py3-none-any.whl", hash = "sha256:0ed14ccfbf1c30a9072c7ca157e4319b70d65f623e91e7b32fadb2853431016e", size = 144225, upload-time = "2024-08-29T15:43:08.921Z" }, ] [[package]]