From d42b5030ae803d4e54b9eaeea3f37921e4aaf2f4 Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 13:41:30 +0800 Subject: [PATCH 1/5] fix: await GitLab approval response and add contract tests --- src/merge_request_hook.py | 15 ++- tests/fixtures/__init__.py | 1 + tests/fixtures/approval_contract.py | 81 +++++++++++++ tests/test_merge_request_approval_contract.py | 113 ++++++++++++++++++ 4 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 tests/fixtures/__init__.py create mode 100644 tests/fixtures/approval_contract.py create mode 100644 tests/test_merge_request_approval_contract.py diff --git a/src/merge_request_hook.py b/src/merge_request_hook.py index 38f2a52..ef1adc6 100644 --- a/src/merge_request_hook.py +++ b/src/merge_request_hook.py @@ -241,15 +241,14 @@ async def check_commit(event, gl): 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 + 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 - if not bot_approved: - await gl.post(f"/projects/{project_id}/merge_requests/{iid}/approve", data=None) + + await gl.post(f"/projects/{project_id}/merge_requests/{iid}/approve", data=None) 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), + ] From 054e5512ebd68bee4f1fb1aa1550b351702639fb Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 13:47:41 +0800 Subject: [PATCH 2/5] T002: await approval before success note --- src/merge_request_hook.py | 2 +- tests/test_merge_request_check_commit.py | 99 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 tests/test_merge_request_check_commit.py diff --git a/src/merge_request_hook.py b/src/merge_request_hook.py index ef1adc6..4cae814 100644 --- a/src/merge_request_hook.py +++ b/src/merge_request_hook.py @@ -224,8 +224,8 @@ 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: + 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)) 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)] From 65638b4b63f5fffd223a5cba75a541aad1b6b160 Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 14:07:24 +0800 Subject: [PATCH 3/5] T003: revoke bot approval on validation failure --- README.md | 2 + src/locales/en/LC_MESSAGES/gitlab-bot.mo | Bin 2263 -> 2833 bytes src/locales/en/LC_MESSAGES/gitlab-bot.po | 22 ++- src/locales/zh/LC_MESSAGES/gitlab-bot.mo | Bin 2230 -> 2789 bytes src/locales/zh/LC_MESSAGES/gitlab-bot.po | 24 ++- src/merge_request_hook.py | 44 +++-- tests/test_merge_request_failure_state.py | 188 ++++++++++++++++++++++ 7 files changed, 267 insertions(+), 13 deletions(-) create mode 100644 tests/test_merge_request_failure_state.py diff --git a/README.md b/README.md index 0c11340..382b822 100644 --- a/README.md +++ b/README.md @@ -221,6 +221,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/src/locales/en/LC_MESSAGES/gitlab-bot.mo b/src/locales/en/LC_MESSAGES/gitlab-bot.mo index 99c47c7f22a2959598ef1a4f610c36c56c3e45f1..9ad5d629a417543850b5084f6d3280bbfcddfced 100644 GIT binary patch delta 830 zcmajc&r1|x7{Kvo+np@6{FU0$F1~v(5z>N=QitwB9=wIa=)CIU?#wptta}Jqf}mr> z7DROoZ$f$sym=5j=|#bV2;B-n|AW3WI|~Ynz3eledEfVW-}jk!kNQ3i)ld2}pM`dh z$PwE_o_LkwLEFc>c!0flilcauIDA8-K>t~yjs5iBVirH)ecVXKcX5dRF%F8<<)Zl_ z9qH!5c^oHUDp4be@&O0%2SywG!YoDX<6ruJG9r_BXo)PLbyH*(%Q%BOIERBB6pJBF zbG~fy@|6L(CGs4;VYI<9zQ-50xeeS)9P1PrrvDE4$#)(Pa2-c*2Pg11QeRv&#H}Zh znw#w=#$VCyHCg}9sCw3l=hwWgFR9n*0qst9zR^M8 z2XR delta 436 zcmX}o&r1S96u|N4uBP_OQreOd=N272b_wbaC^~r!V#KAOaz$gYL+sH64~2B-$DvyY z-aL2p-XXfxHHd=x3;d2QK6v|iGdsMuvp3HBV(^iU-U_imu91)AvZ*6Hhz8E%1)4Kg zn8rKIVh>kv5bl4X!}=eWFlmj=V~+JEZeVe2AeZ5RXUy>81x|+D(aezChr$mnM37eRRiyYu}=nt9;?j}U0B@ii5tg>;2d2C?;yU6@W%uOnq zQZr9%k~uUInITQ2Nb|>LNoI|LZytsDgGrJ*G1xml?7T(R`)$j$b9JRFWu>dtn&<0N Yb*$^EF|9KiA4w!4}3&p%lyi4{mh!CpE95p)iOpi75pm}m-Hn;B+ZJw)tUCnK`d z?GL*LmsW_PC0wvvwQE=dIQuJbLRpe=dsIgWrDMoBh4t`@NZ&7Pmd~y)~8- z#sT68F;8?6S0nr|7I7agVKe@~9$X7Ov_s?=>+_)+wz3|^4xGTlSPq|m!Go;7VY^61 z)`E#NM1zC7@FWT6LJcHQMzIa2&?_*D9W=3stE_*;L=yNlE|SF9PLW=;@gjc4%h=c; zat5#A1?U{#eo^nKSCnRje*9Oz7pdjHel90mR9Xmc(4*`jywB&4@pL4Sxuy5@ zr!?KR9cxhU*Gy|bvmGO4m;-Wyxnm6W8AI9@S^Q%qjhowK{XZjZ#*?NsWL92A>}cE1 z?1QFzhT~X{cGpOy^xMY$@!zrT-n=Tjbw{7opL|e-s{86qty*=8rrTJNMCNYCsSVH!n*21(UF(mer zO;}%ZBuq>#5%Xg6guO<^nMY+_FhsbMl)cQQE8o6kt!!P`PXD9qHR|oVs`vELE;YQH NV&y*&t)+uSdkC>GG7|s* 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 4cae814..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,33 +225,56 @@ 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") 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 approval_merge_request(project_id, iid, gl): +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 + return True + return False + + +async def approval_merge_request(project_id, iid, gl): + 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): if event.data["event_type"] == "merge_request": merge_request_state = event.data["object_attributes"]["state"] 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"), + ] From 6fcae20cd3c13d427802b8fba29a4f91f0a6390b Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 14:11:38 +0800 Subject: [PATCH 4/5] T004: add webhook approval regression tests --- .../test_merge_request_webhook_regression.py | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 tests/test_merge_request_webhook_regression.py 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)] From eb879f345957add1340083ad256d67d3499d3a92 Mon Sep 17 00:00:00 2001 From: zhanglei Date: Sun, 2 Aug 2026 15:19:49 +0800 Subject: [PATCH 5/5] Add run target, fix gidgetlab deprecation warnings, and constrain deps --- Makefile | 7 ++++++- README.md | 3 ++- gitlab_bot.py | 23 ++++++++++++++++++++++- pyproject.toml | 2 ++ tests/test_runtime_startup.py | 4 ++++ uv.lock | 19 ++++++++++++++++--- 6 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 tests/test_runtime_startup.py 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 382b822..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 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/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]]