From 5096dd87808f852498b4fec36a116806da89e2f1 Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 5 Apr 2026 14:08:27 +0300 Subject: [PATCH 1/2] fix: correct label parsing for hyphenated GitHub usernames Label parsing used split("-")[-1] which broke GitHub usernames containing hyphens (e.g., "Ahmad-Hafe" was parsed as "Hafe"). Fixed by using prefix-length slicing to extract the username after the known label prefix. Also switched substring `in` checks to `startswith` for stricter prefix matching. --- .../libs/handlers/pull_request_handler.py | 19 +-- .../tests/test_pull_request_handler.py | 120 ++++++++++++++++++ 2 files changed, 130 insertions(+), 9 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 580f9ad8..fd844642 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1593,16 +1593,17 @@ async def _check_if_pr_approved(self, labels: list[str]) -> str: if self.github_webhook.minimum_lgtm: for _label in labels: - reviewer = _label.split(LABELS_SEPARATOR)[-1] - if LGTM_BY_LABEL_PREFIX.lower() in _label.lower() and reviewer in all_reviewers_without_pr_owner: - lgtm_count += 1 - if reviewer in all_reviewers_without_pr_owner_and_lgtmed: - all_reviewers_without_pr_owner_and_lgtmed.remove(reviewer) + if _label.lower().startswith(LGTM_BY_LABEL_PREFIX.lower()): + reviewer = _label[len(LGTM_BY_LABEL_PREFIX) :] + if reviewer in all_reviewers_without_pr_owner: + lgtm_count += 1 + if reviewer in all_reviewers_without_pr_owner_and_lgtmed: + all_reviewers_without_pr_owner_and_lgtmed.remove(reviewer) self.logger.debug(f"{self.log_prefix} lgtm_count: {lgtm_count}") for _label in labels: - if APPROVED_BY_LABEL_PREFIX.lower() in _label.lower(): - approved_by.append(_label.split(LABELS_SEPARATOR)[-1]) + if _label.lower().startswith(APPROVED_BY_LABEL_PREFIX.lower()): + approved_by.append(_label[len(APPROVED_BY_LABEL_PREFIX) :]) self.logger.debug(f"{self.log_prefix} approved_by: {approved_by}") missing_approvers = list(set(self.owners_file_handler.all_pull_request_approvers.copy())) @@ -1658,8 +1659,8 @@ def _check_labels_for_can_be_merged(self, labels: list[str]) -> str: failure_output = "" for _label in labels: - if CHANGED_REQUESTED_BY_LABEL_PREFIX.lower() in _label.lower(): - change_request_user = _label.split(LABELS_SEPARATOR)[-1] + if _label.lower().startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX.lower()): + change_request_user = _label[len(CHANGED_REQUESTED_BY_LABEL_PREFIX) :] if change_request_user in self.owners_file_handler.all_pull_request_approvers: failure_output += "PR has changed requests from approvers\n" self.logger.debug(f"{self.log_prefix} Found changed request by {change_request_user}") diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 0168dd3b..1b83cb43 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -1680,6 +1680,64 @@ async def test_check_if_pr_approved_commented(self, pull_request_handler: PullRe result = await pull_request_handler._check_if_pr_approved(labels=[f"{COMMENTED_BY_LABEL_PREFIX}reviewer1"]) assert result == "" # Empty string means no errors + @pytest.mark.asyncio + async def test_check_if_pr_approved_lgtm_hyphenated_username( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that LGTM labels with hyphenated usernames are parsed correctly.""" + hyphenated_user = "Ahmad-Hafe" + with ( + patch.object( + pull_request_handler.owners_file_handler, + "owners_data_for_changed_files", + _owners_data_coroutine(), + ), + patch.object(pull_request_handler.github_webhook, "minimum_lgtm", 1), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", []), + patch.object(pull_request_handler.owners_file_handler, "root_approvers", []), + patch.object(pull_request_handler.owners_file_handler, "root_reviewers", []), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_reviewers", [hyphenated_user]), + patch.object(pull_request_handler.github_webhook, "parent_committer", "someone-else"), + ): + result = await pull_request_handler._check_if_pr_approved( + labels=[f"{LGTM_BY_LABEL_PREFIX}{hyphenated_user}"] + ) + assert result == "" # Hyphenated username should be recognized as a valid reviewer + + @pytest.mark.asyncio + async def test_check_if_pr_approved_approved_hyphenated_username( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that approved labels with hyphenated usernames are parsed correctly.""" + hyphenated_user = "Ahmad-Hafe" + with ( + patch.object( + pull_request_handler.owners_file_handler, + "owners_data_for_changed_files", + _owners_data_coroutine(), + ), + patch.object(pull_request_handler.github_webhook, "minimum_lgtm", 0), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", [hyphenated_user]), + patch.object(pull_request_handler.owners_file_handler, "root_approvers", [hyphenated_user]), + patch.object(pull_request_handler.owners_file_handler, "root_reviewers", []), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_reviewers", []), + ): + result = await pull_request_handler._check_if_pr_approved( + labels=[f"{APPROVED_BY_LABEL_PREFIX}{hyphenated_user}"] + ) + assert result == "" # Hyphenated username should be recognized as an approver + + def test_check_labels_for_can_be_merged_changes_requested_hyphenated_username( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that changes-requested labels with hyphenated usernames are parsed correctly.""" + hyphenated_user = "Ahmad-Hafe" + with patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", [hyphenated_user]): + result = pull_request_handler._check_labels_for_can_be_merged( + labels=[f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}{hyphenated_user}"] + ) + assert "PR has changed requests from approvers" in result + def test_check_labels_for_can_be_merged_approved(self, pull_request_handler: PullRequestHandler) -> None: # Mock the logic to return empty string (no errors) when appropriate with patch.object(pull_request_handler, "_check_if_pr_approved", return_value=""): @@ -1710,6 +1768,68 @@ def test_check_labels_for_can_be_merged_not_approved(self, pull_request_handler: result = pull_request_handler._check_labels_for_can_be_merged(labels=["other-label"]) assert result == "" # Empty string means no errors + @pytest.mark.asyncio + async def test_check_if_pr_approved_lgtm_label_only_matches_prefix( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that only labels starting with LGTM prefix are counted as LGTM.""" + reviewer = "reviewer1" + with ( + patch.object( + pull_request_handler.owners_file_handler, + "owners_data_for_changed_files", + _owners_data_coroutine(), + ), + patch.object(pull_request_handler.github_webhook, "minimum_lgtm", 1), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", []), + patch.object(pull_request_handler.owners_file_handler, "root_approvers", []), + patch.object(pull_request_handler.owners_file_handler, "root_reviewers", []), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_reviewers", [reviewer]), + patch.object(pull_request_handler.github_webhook, "parent_committer", "someone-else"), + ): + # Valid LGTM label starting with prefix should count + result = await pull_request_handler._check_if_pr_approved(labels=[f"{LGTM_BY_LABEL_PREFIX}{reviewer}"]) + assert result == "" + + # Label containing prefix in the middle should NOT count + result = await pull_request_handler._check_if_pr_approved(labels=[f"not-{LGTM_BY_LABEL_PREFIX}{reviewer}"]) + assert "Missing lgtm from reviewers" in result + + def test_check_labels_for_can_be_merged_approved_label_only_matches_prefix( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that only labels starting with approved prefix are matched.""" + with patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", ["approver1"]): + # Valid approved label should not trigger changes-requested + result = pull_request_handler._check_labels_for_can_be_merged( + labels=[f"{APPROVED_BY_LABEL_PREFIX}approver1"] + ) + assert result == "" + + # Label containing approved prefix in the middle should not match + result = pull_request_handler._check_labels_for_can_be_merged( + labels=[f"not-{APPROVED_BY_LABEL_PREFIX}approver1"] + ) + assert result == "" + + def test_check_labels_for_can_be_merged_changes_requested_label_only_matches_prefix( + self, pull_request_handler: PullRequestHandler + ) -> None: + """Test that only labels starting with changes-requested prefix are matched.""" + approver = "reviewer1" + with patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", [approver]): + # Valid changes-requested label should trigger the check + result = pull_request_handler._check_labels_for_can_be_merged( + labels=[f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}{approver}"] + ) + assert "PR has changed requests from approvers" in result + + # Label containing prefix in the middle should NOT trigger + result = pull_request_handler._check_labels_for_can_be_merged( + labels=[f"not-{CHANGED_REQUESTED_BY_LABEL_PREFIX}{approver}"] + ) + assert result == "" + @pytest.mark.asyncio async def test_skip_if_pull_request_already_merged_merged( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock From 4ba81dd4b5f37893e539c836a9767fd50ed5962f Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 5 Apr 2026 14:21:28 +0300 Subject: [PATCH 2/2] test: fix approved-prefix test to target correct code path --- .../tests/test_pull_request_handler.py | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 1b83cb43..0f2263d0 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -1795,22 +1795,31 @@ async def test_check_if_pr_approved_lgtm_label_only_matches_prefix( result = await pull_request_handler._check_if_pr_approved(labels=[f"not-{LGTM_BY_LABEL_PREFIX}{reviewer}"]) assert "Missing lgtm from reviewers" in result - def test_check_labels_for_can_be_merged_approved_label_only_matches_prefix( + @pytest.mark.asyncio + async def test_check_if_pr_approved_approved_label_only_matches_prefix( self, pull_request_handler: PullRequestHandler ) -> None: """Test that only labels starting with approved prefix are matched.""" - with patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", ["approver1"]): - # Valid approved label should not trigger changes-requested - result = pull_request_handler._check_labels_for_can_be_merged( - labels=[f"{APPROVED_BY_LABEL_PREFIX}approver1"] - ) + approver = "approver1" + with ( + patch.object( + pull_request_handler.owners_file_handler, + "owners_data_for_changed_files", + _owners_data_coroutine({"repo/OWNERS": {"approvers": [approver]}}), + ), + patch.object(pull_request_handler.github_webhook, "minimum_lgtm", 0), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_approvers", [approver]), + patch.object(pull_request_handler.owners_file_handler, "root_approvers", []), + patch.object(pull_request_handler.owners_file_handler, "root_reviewers", []), + patch.object(pull_request_handler.owners_file_handler, "all_pull_request_reviewers", []), + ): + result = await pull_request_handler._check_if_pr_approved(labels=[f"{APPROVED_BY_LABEL_PREFIX}{approver}"]) assert result == "" - # Label containing approved prefix in the middle should not match - result = pull_request_handler._check_labels_for_can_be_merged( - labels=[f"not-{APPROVED_BY_LABEL_PREFIX}approver1"] + result = await pull_request_handler._check_if_pr_approved( + labels=[f"not-{APPROVED_BY_LABEL_PREFIX}{approver}"] ) - assert result == "" + assert "Missing approved from approvers" in result def test_check_labels_for_can_be_merged_changes_requested_label_only_matches_prefix( self, pull_request_handler: PullRequestHandler