Skip to content

fix: correct label parsing for hyphenated GitHub usernames#1061

Merged
myakove merged 2 commits intomainfrom
fix/hyphenated-username-label-parsing
Apr 5, 2026
Merged

fix: correct label parsing for hyphenated GitHub usernames#1061
myakove merged 2 commits intomainfrom
fix/hyphenated-username-label-parsing

Conversation

@rnetser
Copy link
Copy Markdown
Collaborator

@rnetser rnetser commented Apr 5, 2026

Summary

  • Label parsing used split("-")[-1] which broke GitHub usernames containing hyphens (e.g., Ahmad-Hafe was parsed as Hafe), causing LGTM/approved/changes-requested labels to not be recognized
  • Fixed by using prefix-length slicing (_label[len(PREFIX):]) to correctly extract the full username
  • Switched substring in checks to startswith for consistency with the prefix-based extraction and other patterns in the file

Bug Details

The can-be-merged check counted only 1 LGTM instead of 2 when one reviewer had a hyphenated username. For example, label lgtm-Ahmad-Hafe was split as ["lgtm", "Ahmad", "Hafe"] and only "Hafe" was extracted — which matched no reviewer.

Affected all three label types: lgtm-, approved-, changes-requested-.

Bug existed since commit c90382a (2023-05-24).

Test plan

  • Added tests for hyphenated usernames across all 3 label types (lgtm, approved, changes-requested)
  • Added tests verifying substring labels are not falsely matched (startswith fix)
  • All 1491 tests pass
  • 90.62% coverage (meets 90% threshold)

Summary by CodeRabbit

  • Bug Fixes

    • More robust label parsing for pull request approvals so LGTM, approval, and change-request labels correctly recognize hyphenated usernames and avoid false matches.
  • Tests

    • Added unit tests covering label parsing with hyphenated usernames and prefix-matching behavior to prevent regressions.

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.
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix label parsing for hyphenated GitHub usernames

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Fixed label parsing for GitHub usernames containing hyphens
  - Changed from split("-")[-1] to prefix-length slicing
  - Affected LGTM, approved, and changes-requested labels
• Replaced substring in checks with startswith for stricter prefix matching
• Added comprehensive tests for hyphenated usernames and prefix-only matching
Diagram
flowchart LR
  A["Label with hyphen<br/>lgtm-Ahmad-Hafe"] -->|Old: split by dash| B["Extracted only<br/>Hafe"]
  A -->|New: prefix slicing| C["Extracted full<br/>Ahmad-Hafe"]
  B --> D["Username not found<br/>in reviewers"]
  C --> E["Username matched<br/>correctly"]
  F["Substring in check<br/>lgtm in not-lgtm-user"] -->|Old: True| G["False positive"]
  F -->|New: startswith| H["False negative<br/>prevented"]
Loading

Grey Divider

File Changes

1. webhook_server/libs/handlers/pull_request_handler.py 🐞 Bug fix +10/-9

Replace split-based parsing with prefix-length slicing

• Fixed LGTM label parsing by replacing split(LABELS_SEPARATOR)[-1] with
 _label[len(LGTM_BY_LABEL_PREFIX):]
• Changed substring in check to startswith() for LGTM prefix matching
• Applied same fix to approved label parsing with startswith() and prefix-length slicing
• Fixed changes-requested label parsing with startswith() and prefix-length slicing

webhook_server/libs/handlers/pull_request_handler.py


2. webhook_server/tests/test_pull_request_handler.py 🧪 Tests +120/-0

Add tests for hyphenated usernames and prefix matching

• Added test for LGTM labels with hyphenated usernames (Ahmad-Hafe)
• Added test for approved labels with hyphenated usernames
• Added test for changes-requested labels with hyphenated usernames
• Added tests verifying substring labels are not falsely matched (prefix-only matching)

webhook_server/tests/test_pull_request_handler.py


Grey Divider

Qodo Logo

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3fc30e9e-6a5b-4b4b-a06b-d3aed47deee1

📥 Commits

Reviewing files that changed from the base of the PR and between 5096dd8 and 4ba81dd.

📒 Files selected for processing (1)
  • webhook_server/tests/test_pull_request_handler.py

Walkthrough

Refactored label parsing in the pull request handler to use prefix-based matching (.startswith()) and suffix slicing for extracting usernames from LGTM, APPROVED_BY, and CHANGED_REQUESTED_BY labels. Added unit tests covering hyphenated usernames and prefix-matching semantics.

Changes

Cohort / File(s) Summary
Label Parsing Refactor
webhook_server/libs/handlers/pull_request_handler.py
Replaced split(LABELS_SEPARATOR) + containment checks with lower().startswith(<PREFIX>) and suffix extraction via len(<PREFIX>) in _check_if_pr_approved and _check_labels_for_can_be_merged to strictly extract reviewer/approver usernames.
Test Coverage
webhook_server/tests/test_pull_request_handler.py
Added async/sync tests validating LGTM, APPROVED_BY, and CHANGED_REQUESTED_BY parsing for hyphenated GitHub usernames and verifying that only labels starting with the exact prefixes match (labels containing prefixes mid-string do not).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

size/S, can-be-merged

Suggested reviewers

  • myakove
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: correct label parsing for hyphenated GitHub usernames' directly and specifically summarizes the main change: fixing label parsing to properly handle hyphenated usernames instead of truncating them.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hyphenated-username-label-parsing

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 5, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@myakove-bot
Copy link
Copy Markdown
Collaborator

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: Disabled for this repository
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest python-module-install - Test Python package installation
  • /retest pre-commit - Run pre-commit hooks and checks
  • /retest conventional-title - Validate commit message format
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 1 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove
  • rnetser

Reviewers:

  • myakove
  • rnetser
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
  • Test Oracle: Triggers: approved (claude/claude-opus-4-6[1m]); /test-oracle can be used anytime

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@myakove
Copy link
Copy Markdown
Collaborator

myakove commented Apr 5, 2026

/lgtm
/approve

Comment thread webhook_server/libs/handlers/pull_request_handler.py
@myakove myakove enabled auto-merge (squash) April 5, 2026 11:16
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@webhook_server/tests/test_pull_request_handler.py`:
- Around line 1798-1814: The test is exercising the wrong code path:
approved-label parsing lives in _check_if_pr_approved, not
_check_labels_for_can_be_merged; update the
test_test_check_labels_for_can_be_merged_approved_label_only_matches_prefix to
call pull_request_handler._check_if_pr_approved (passing the same labels list)
and keep patching
pull_request_handler.owners_file_handler.all_pull_request_approvers to
["approver1"] and assertions unchanged so the test actually verifies that labels
starting with APPROVED_BY_LABEL_PREFIX match while labels containing the prefix
in the middle do not.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: af0876c9-bf72-47cc-850e-d8784676ca05

📥 Commits

Reviewing files that changed from the base of the PR and between 6c55487 and 5096dd8.

📒 Files selected for processing (2)
  • webhook_server/libs/handlers/pull_request_handler.py
  • webhook_server/tests/test_pull_request_handler.py

Comment thread webhook_server/tests/test_pull_request_handler.py Outdated
@rnetser
Copy link
Copy Markdown
Collaborator Author

rnetser commented Apr 5, 2026

/build-and-push-container

@myakove-bot
Copy link
Copy Markdown
Collaborator

New container for ghcr.io/myk-org/github-webhook-server:pr-1061 published

@rnetser
Copy link
Copy Markdown
Collaborator Author

rnetser commented Apr 5, 2026

/verified

@myakove
Copy link
Copy Markdown
Collaborator

myakove commented Apr 5, 2026

/approve
/lgtm

@myakove myakove merged commit 2d3f773 into main Apr 5, 2026
9 checks passed
@myakove myakove deleted the fix/hyphenated-username-label-parsing branch April 5, 2026 11:41
Comment thread webhook_server/libs/handlers/pull_request_handler.py
@myakove-bot
Copy link
Copy Markdown
Collaborator

Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-1061.

@myakove-bot
Copy link
Copy Markdown
Collaborator

New container for ghcr.io/myk-org/github-webhook-server:latest published

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants