fix: prevent has-conflicts label flapping when mergeable is None#1000
Open
fix: prevent has-conflicts label flapping when mergeable is None#1000
Conversation
When GitHub is still computing merge status, pull_request.mergeable returns None. Previously this was treated as "no conflicts", causing the has-conflicts label to be incorrectly removed and then re-added once GitHub finished computing — resulting in label flapping. Now polls mergeable using TimeoutSampler (wait_timeout=15, sleep=3) wrapped in asyncio.to_thread() until GitHub returns a definitive True/False. Only skips the label update if still None after retries. Closes #999
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Review Summary by QodoPrevent has-conflicts label flapping when mergeable is None
WalkthroughsDescription• Prevent has-conflicts label flapping by polling mergeable status • Use TimeoutSampler to wait for GitHub's merge computation • Wrap polling in asyncio.to_thread to avoid blocking event loop • Skip label updates only when mergeable remains None after retries Diagramflowchart LR
A["PR webhook received"] --> B["Check mergeable status"]
B --> C{mergeable is None?}
C -->|Yes| D["Poll with TimeoutSampler<br/>15s timeout, 3s interval"]
D --> E{Got definitive<br/>True/False?}
E -->|Yes| F["Update labels based<br/>on merge state"]
E -->|No| G["Skip label update<br/>return early"]
C -->|No| F
F --> H["Complete"]
G --> H
File Changes1. webhook_server/libs/handlers/pull_request_handler.py
|
Code Review by Qodo
1.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds polling logic to resolve Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The implementation introduces new polling logic with exception handling in a critical code path, and the test suite expansion requires careful validation that all timeout and polling scenarios are properly covered. Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/libs/handlers/pull_request_handler.py`:
- Around line 975-982: The polling function _poll_mergeable currently calls
repository.get_pull(pr_number).mergeable directly; change it to use the unified
API on the webhook object (self.github_webhook.unified_api) to fetch the pull
and its mergeable status inside TimeoutSampler (replace the lambda that calls
repository.get_pull with one that calls
self.github_webhook.unified_api.<appropriate_method>(pr_number) and reads
.mergeable or returns the unified API result), ensuring all GitHub operations in
_poll_mergeable and the surrounding handler use self.github_webhook.unified_api
rather than direct repository/PyGithub access.
- Around line 970-997: The poll function _poll_mergeable can return None but the
code only handles exceptions; after awaiting asyncio.to_thread(_poll_mergeable)
add an explicit guard: if mergeable is None then log a warning (same message
used in the except block), call self.ctx.complete_step("label_merge_state",
mergeable_unknown=True) when self.ctx is set, and return early to avoid
computing has_conflicts and clearing the conflict label; this ensures mergeable
is only used when not None.
In `@webhook_server/tests/test_pull_request_handler.py`:
- Around line 2322-2380: Add tests that cover the success polling path of
label_pull_request_by_merge_state: create one test where
mock_pull_request.mergeable starts as None and the patched TimeoutSampler yields
a definitive True (mergeable) and assert that _add_label/_remove_label are
called appropriately, and a second (or parametrized) test where TimeoutSampler
yields False (not mergeable) and assert the opposite labeling behavior; patch
pull_request_handler.labels_handler.pull_request_labels_names, _add_label and
_remove_label like the existing tests and patch
"webhook_server.libs.handlers.pull_request_handler.TimeoutSampler" to return an
iterator/sequence of values (first raising no result then returning True/False)
so the handler observes mergeable change and proceeds to call the right label
methods.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_pull_request_handler.py
- Catch TimeoutExpiredError specifically instead of broad Exception - Re-raise asyncio.CancelledError to preserve task cancellation - Use logger.exception() for unexpected errors (includes traceback) - Add post-poll None guard to prevent label flapping on edge cases - Add tests for polling resolving to True (mergeable) and False (conflicts)
myakove-bot
added
size/L
and removed
changes-requested-coderabbitai[bot]
commented-qodo-code-review[bot]
size/M
labels
There was a problem hiding this comment.
♻️ Duplicate comments (1)
webhook_server/libs/handlers/pull_request_handler.py (1)
974-982:⚠️ Potential issue | 🟠 MajorHIGH: Polling still uses direct PyGithub access in handler code.
Line 981 calls
repository.get_pull(pr_number).mergeabledirectly from the handler path. This bypasses the handler API boundary and can drift behavior (auth/retry/logging) from other handler operations that go throughunified_api.Please route the polling fetch through
self.github_webhook.unified_apiinstead ofself.github_webhook.repository.#!/bin/bash set -euo pipefail # Verify direct pull fetches from handlers rg -nP --type=py '\brepository\.get_pull\s*\(' webhook_server/libs/handlers # Inspect existing unified_api pull-related calls in handlers rg -nP --type=py '\bunified_api\.[A-Za-z_]*pull[A-Za-z_]*\s*\(' webhook_server/libs/handlersExpected result: no direct
repository.get_pull(...)in handler GitHub operation paths.As per coding guidelines: "Handlers must use
self.github_webhook.unified_apifor all GitHub operations, not direct PyGithub calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/libs/handlers/pull_request_handler.py` around lines 974 - 982, The polling currently calls repository.get_pull(pr_number).mergeable directly inside _poll_mergeable/TimeoutSampler which bypasses the unified API; change the lambda to call the unified API method on self.github_webhook.unified_api that returns the PR (or its mergeable status) instead of using repository.get_pull, e.g. invoke the existing unified_api pull-fetch method (use the same method name used elsewhere on unified_api for pull retrieval) and read its .mergeable field inside _poll_mergeable so all GitHub operations flow through self.github_webhook.unified_api.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@webhook_server/libs/handlers/pull_request_handler.py`:
- Around line 974-982: The polling currently calls
repository.get_pull(pr_number).mergeable directly inside
_poll_mergeable/TimeoutSampler which bypasses the unified API; change the lambda
to call the unified API method on self.github_webhook.unified_api that returns
the PR (or its mergeable status) instead of using repository.get_pull, e.g.
invoke the existing unified_api pull-fetch method (use the same method name used
elsewhere on unified_api for pull retrieval) and read its .mergeable field
inside _poll_mergeable so all GitHub operations flow through
self.github_webhook.unified_api.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_pull_request_handler.py
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:pr-1000 published |
…fix/has-conflicts-label-flapping
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
webhook_server/libs/handlers/pull_request_handler.py (1)
1001-1008:⚠️ Potential issue | 🟠 MajorHIGH: Use
unified_apifor polling fetch instead of direct repository access.Line 1007 uses
repository.get_pull(pr_number).mergeabledirectly inside handler logic. That bypasses the handler abstraction and makes API access inconsistent in this layer; route this throughself.github_webhook.unified_apiinstead.As per coding guidelines: "Handler implementations must use
self.github_webhook.unified_apifor all GitHub API operations".#!/bin/bash # Verify direct pull-fetch calls in handlers and discover unified_api pull helpers. echo "== Direct repository.get_pull usage in handlers ==" rg -nP --type=py -C2 '\brepository\.get_pull\s*\(' webhook_server/libs/handlers echo echo "== unified_api usage in libs ==" rg -nP --type=py -C2 '\bunified_api\.' webhook_server/libs echo echo "== Candidate unified_api pull-related methods ==" rg -nP --type=py -C2 'def\s+.*pull.*\(' webhook_server/libs/github_api.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/libs/handlers/pull_request_handler.py` around lines 1001 - 1008, The handler currently polls mergeability by calling repository.get_pull(pr_number).mergeable inside _poll_mergeable (via TimeoutSampler), which bypasses the handler abstraction; change _poll_mergeable to call the unified API instead (use self.github_webhook.unified_api's pull-fetch helper — e.g., a get_pull or fetch_pull_mergeable method) and read the mergeable value from that response; if no suitable unified_api helper exists, add a small wrapper on self.github_webhook.unified_api (e.g., unified_api.get_pull(pr_number) or unified_api.fetch_pull(pr_number)) and use that in _poll_mergeable so all GitHub access goes through self.github_webhook.unified_api rather than repository.get_pull.
🤖 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/libs/handlers/pull_request_handler.py`:
- Around line 1024-1028: The inner except block in pull_request_handler.py
currently both logs the error and calls self.ctx.fail_step, which duplicates the
outer handler’s failure recording; remove the self.ctx.fail_step(...) call from
the inner except block (keep the existing self.logger.exception(...) and
re-raise) so only the outer exception handler records step failure; ensure any
references to traceback.format_exc() or the ctx guard (if self.ctx) are deleted
along with the fail_step call.
---
Duplicate comments:
In `@webhook_server/libs/handlers/pull_request_handler.py`:
- Around line 1001-1008: The handler currently polls mergeability by calling
repository.get_pull(pr_number).mergeable inside _poll_mergeable (via
TimeoutSampler), which bypasses the handler abstraction; change _poll_mergeable
to call the unified API instead (use self.github_webhook.unified_api's
pull-fetch helper — e.g., a get_pull or fetch_pull_mergeable method) and read
the mergeable value from that response; if no suitable unified_api helper
exists, add a small wrapper on self.github_webhook.unified_api (e.g.,
unified_api.get_pull(pr_number) or unified_api.fetch_pull(pr_number)) and use
that in _poll_mergeable so all GitHub access goes through
self.github_webhook.unified_api rather than repository.get_pull.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_pull_request_handler.py
The inner except block's ctx.fail_step() duplicated the outer method-level handler's failure recording. Let the outer handler own step-failure recording for a single source of truth.
…fix/has-conflicts-label-flapping
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:pr-1000 published |
…fix/has-conflicts-label-flapping
Poll mergeable status using TimeoutSampler (30s timeout, 5s interval) when GitHub returns None. Guard has-conflicts update with mergeable None check to prevent removing label on unknown state. Exit early when conflicts confirmed (skip needs-rebase check).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
has-conflictslabel flapping caused by GitHub returning stale or unknownmergeablestatus after base branch changes.Flow
Root Cause
pull_request.mergeablereturnsNonewhile GitHub computes merge statusNoneas "no conflicts" (None is False→False), removinghas-conflictslabelmergeable=Falseand re-adds it → flappingChanges
mergeablewithTimeoutSampler(30s/5s) wrapped inasyncio.to_thread()until definitiveTrue/Falseif mergeable is not None:— unknown status preserves current labelasyncio.CancelledError, catchTimeoutExpiredErrorspecificallyTest plan