Skip to content

fix: correct label parsing for hyphenated GitHub usernames #1061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
myakove merged 2 commits into from
Apr 5, 2026
Merged

fix: correct label parsing for hyphenated GitHub usernames #1061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5096dd8
fix: correct label parsing for hyphenated GitHub usernames
rnetser Apr 5, 2026
4ba81dd
test: fix approved-prefix test to target correct code path
rnetser Apr 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions webhook_server/libs/handlers/pull_request_handler.py
View file
Open in desktop
Comment thread
rnetser marked this conversation as resolved.
Comment thread
myakove marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down Expand Up @@ -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}")
Expand Down
129 changes: 129 additions & 0 deletions webhook_server/tests/test_pull_request_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=""):
Expand Down Expand Up @@ -1710,6 +1768,77 @@ 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

@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."""
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 == ""

result = await pull_request_handler._check_if_pr_approved(
labels=[f"not-{APPROVED_BY_LABEL_PREFIX}{approver}"]
)
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
) -> 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
Expand Down