fix: re-evaluate can-be-merged when review threads are resolved (#1038)#1039
Merged
fix: re-evaluate can-be-merged when review threads are resolved (#1038)#1039
Conversation
Add pull_request_review_thread event handler so resolved/unresolved thread actions trigger can-be-merged re-evaluation via GraphQL API.
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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds handling for Changes
Sequence DiagramsequenceDiagram
participant GitHub as GitHub (Webhook)
participant Server as Webhook Server
participant Repo as Repository (local/git)
participant Owners as OwnersFileHandler
participant PRH as PullRequestHandler
participant API as GitHub API
GitHub->>Server: deliver event (pull_request_review_thread / status / other)
Server->>Server: parse event, evaluate early-exit (action/state, config)
alt re-evaluation required
Server->>Repo: clone repository (deferred inside recheck)
Repo-->>Server: clone complete
Server->>Owners: OwnersFileHandler.initialize()
Owners-->>Server: owners loaded
Server->>PRH: _recheck_merge_eligibility(pr) -> check_if_can_be_merged(pr)
PRH->>API: GraphQL/REST (threads, checks, commit statuses, owners)
API-->>PRH: current mergeability state
PRH-->>Server: mergeability result
Server->>Server: log token metrics
end
Server-->>GitHub: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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/github_api.py`:
- Around line 653-656: The OwnersFileHandler is constructed but not initialized
before being passed into PullRequestHandler.check_if_can_be_merged, so call
await OwnersFileHandler.initialize(pull_request=pull_request) (or await
owners_file_handler.initialize(pull_request=pull_request)) after creating
owners_file_handler and before invoking
PullRequestHandler(...).check_if_can_be_merged to ensure the
approver/reviewer/OWNERS state is loaded for the
PullRequestHandler.check_if_can_be_merged logic.
- Around line 640-649: The action check for pull_request_review_thread must be
moved earlier to avoid performing GitHub I/O for ignored actions: in the handler
that reads self.hook_data["action"] (the pull_request_review_thread webhook
handler), hoist the guard that checks if action not in ("resolved","unresolved")
to run before any PR fetching or pull_request.get_commits() calls; keep the
existing token_metrics logging and the await self._update_context_metrics() and
return None in that early branch so skipped actions perform only lightweight
local work and no GitHub API calls.
In `@webhook_server/tests/test_github_api.py`:
- Around line 2263-2272: The test currently fully mocks PullRequestHandler so it
only verifies check_if_can_be_merged was awaited and misses whether
OwnersFileHandler.initialize() was called; update the test to also assert
OwnersFileHandler.initialize() is awaited (or replace the full
PullRequestHandler mock with a stricter fake that calls through to
OwnersFileHandler initialization) by patching or spying on
OwnersFileHandler.initialize and adding an assert (e.g.,
OwnersFileHandler.initialize.assert_awaited_once() when should_recheck is true
and assert_not_awaited() when false) while keeping the existing
PullRequestHandler and GithubWebhook.process usage.
🪄 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: 287f47d3-291a-4537-8655-c73d2d55617e
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
- Early return for non-actionable thread events before PR fetch - Clone repo and initialize OwnersFileHandler for OWNERS data - Test asserts initialize() and clone called for resolved/unresolved
myakove-bot
added
size/L
and removed
commented-myakove
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
size/M
labels
- Add clone + OwnersFileHandler.initialize for status event handler - Update webhook if configured events differ from existing - Add inline comments explaining why clone is needed - Add pull_request_review_thread and status to example configs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/libs/github_api.py (1)
640-680: 🧹 Nitpick | 🔵 TrivialLOW: Consider extracting a shared mergeability recheck helper to remove duplication.
Line 640–Line 680 duplicates the same recheck workflow across
statusandpull_request_review_thread. A small helper would reduce maintenance drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/libs/github_api.py` around lines 640 - 680, Extract the duplicated recheck workflow into a private helper (e.g., _recheck_mergeability or similar) and call it from both the "status" and "pull_request_review_thread" branches; the helper should perform the sequence currently duplicated: await self._clone_repository(pull_request), instantiate and initialize OwnersFileHandler (OwnersFileHandler.initialize(pull_request)), call PullRequestHandler(...).check_if_can_be_merged(pull_request), fetch token_metrics via await self._get_token_metrics(), log the completion message, and await self._update_context_metrics(); replace the duplicated blocks with a single call to that helper to centralize behavior and keep existing logging/context intact.
🤖 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_github_api.py`:
- Around line 2232-2300: The test doesn't assert that skipped review-thread
actions avoid fetching the PR; after calling GithubWebhook(review_thread_data,
headers, logger).process(), add assertions on the mocked repo PR fetch
(mock_repo.get_pull) to ensure it's not called when should_recheck is False and
is called exactly once when should_recheck is True; place these assertions
alongside the existing checks for mock_pr_handler.check_if_can_be_merged,
mock_owners_instance.initialize, and mock_clone within
test_process_review_thread_event so any regression that still calls get_pull is
caught.
In `@webhook_server/tests/test_webhook.py`:
- Around line 454-457: Add an explicit non-destructive assertion to the “no
update needed” test by asserting that no delete call occurred: after the
existing assertions for existing_hook.edit.assert_not_called() and
mock_repo.create_hook.assert_not_called(), add an assertion such as
mock_repo.delete_hook.assert_not_called() (and if the hook object exposes a
delete method, also existing_hook.delete.assert_not_called()) so the test
verifies no deletion happens in the no-op path.
- Around line 318-457: Several almost-identical tests
(test_process_github_webhook_existing_hook_different_events,
test_process_github_webhook_existing_hook_wildcard_vs_specific_events,
test_process_github_webhook_existing_hook_specific_vs_wildcard_events,
test_process_github_webhook_existing_hook_events_same_order_differs) only vary
the existing hook events, incoming data events and expected outcomes;
consolidate them into a single parameterized pytest function using
pytest.mark.parametrize. Create one test (e.g.,
test_process_github_webhook_event_update_matrix) parameterized over tuples of
(existing_events, data_events, expected_edit_events, expected_message_substring,
expect_edit_call_bool) and inside the test reuse the same mock setup
(existing_hook = Mock(), existing_hook.config, mock_repo.get_hooks.return_value,
mock_get_repo_api.return_value), call process_github_webhook with
repository_name/data/webhook_ip/apis_dict, then assert success and that
expected_message_substring is in message and assert existing_hook.edit called
(or not) with name="web", config={"url":
"http://example.com","content_type":"json"}, events=expected_edit_events,
active=True and that mock_repo.create_hook.assert_not_called(); include cases
for wildcard "*" and reversed-order events to ensure order-insensitivity.
In `@webhook_server/utils/webhook.py`:
- Around line 59-67: The comparison currently uses sorted lists (hook_events and
config_events) which can still differ when config contains duplicate events
causing unnecessary edits; before comparing and before calling _hook.edit,
normalize both event lists by deduplicating (e.g., convert to set) and then sort
to produce canonical hook_events and config_events, and pass the
deduplicated/sorted events into _hook.edit (reference variables: _hook.events,
events, hook_events, config_events and method _hook.edit); update the LOGGER
message to show the normalized lists so logs reflect the actual change.
---
Outside diff comments:
In `@webhook_server/libs/github_api.py`:
- Around line 640-680: Extract the duplicated recheck workflow into a private
helper (e.g., _recheck_mergeability or similar) and call it from both the
"status" and "pull_request_review_thread" branches; the helper should perform
the sequence currently duplicated: await self._clone_repository(pull_request),
instantiate and initialize OwnersFileHandler
(OwnersFileHandler.initialize(pull_request)), call
PullRequestHandler(...).check_if_can_be_merged(pull_request), fetch
token_metrics via await self._get_token_metrics(), log the completion message,
and await self._update_context_metrics(); replace the duplicated blocks with a
single call to that helper to centralize behavior and keep existing
logging/context intact.
🪄 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: 653b4c9f-c2a8-47ff-b088-78012a457e24
📒 Files selected for processing (7)
examples/.github-webhook-server.yamlexamples/config.yamlwebhook_server/libs/github_api.pywebhook_server/tests/manifests/config.yamlwebhook_server/tests/test_github_api.pywebhook_server/tests/test_webhook.pywebhook_server/utils/webhook.py
rnetser
requested changes
Mar 17, 2026
myakove-bot
added
commented-myakove
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
- Track webhook_routing step and metrics for early-exit branches - Assert add_api_users and get_pull_request not called on pending skip
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
webhook_server/tests/test_github_api.py (1)
879-915: 🧹 Nitpick | 🔵 TrivialMEDIUM: The shared recheck tests still don’t prove which owners handler reaches
PullRequestHandler.Line 887 returns
NonefromOwnersFileHandler.initialize(), and Lines 2319-2324 only prove thatcheck_if_can_be_merged()was awaited. BecausePullRequestHandleris fully mocked, both tests would stay green if_recheck_merge_eligibility()awaitedinitialize()and then instantiatedPullRequestHandlerwith the wrong handler. These tests are covering the shared helper now, so they should assert the constructor input directly.Suggested hardening
- with patch.object( - GithubWebhook, - "_clone_repository", - new=AsyncMock(return_value=None), - ) as mock_clone: - with patch.object( - OwnersFileHandler, - "initialize", - new=AsyncMock(return_value=None), - ) as mock_owners_init: + with ( + patch.object( + GithubWebhook, + "_clone_repository", + new=AsyncMock(return_value=None), + ) as mock_clone, + patch("webhook_server.libs.github_api.OwnersFileHandler") as mock_owners_handler, + ): + mock_owners_instance = Mock() + mock_owners_instance.initialize = AsyncMock(return_value=mock_owners_instance) + mock_owners_handler.return_value = mock_owners_instance webhook = GithubWebhook(status_data, headers, logger) @@ - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - mock_owners_init.assert_awaited_once() + mock_pr_handler.assert_called_once_with( + github_webhook=webhook, + owners_file_handler=mock_owners_instance, + ) + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() mock_clone.assert_awaited_once() @@ - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() - mock_owners_init.assert_not_awaited() + mock_pr_handler.assert_not_called() + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() mock_clone.assert_not_awaited()Apply the same
mock_pr_handler.assert_called_once_with(github_webhook=webhook, owners_file_handler=mock_owners_instance)assertion in the review-thread test as well.Also applies to: 2293-2324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/tests/test_github_api.py` around lines 879 - 915, The shared recheck tests only assert that PullRequestHandler.check_if_can_be_merged() was awaited, not that PullRequestHandler was constructed with the correct OwnersFileHandler instance; update the tests around the GithubWebhook.process() calls (where OwnersFileHandler.initialize is patched and mock_pr_handler is used) to also assert that mock_pr_handler.assert_called_once_with(github_webhook=webhook, owners_file_handler=mock_owners_instance) (or equivalent) after awaiting webhook.process(), ensuring PullRequestHandler was instantiated with the actual OwnersFileHandler instance returned by OwnersFileHandler.initialize; apply the same assertion in both the shared recheck test and the review-thread test that cover _recheck_merge_eligibility().
🤖 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/tests/test_github_api.py`:
- Around line 879-915: The shared recheck tests only assert that
PullRequestHandler.check_if_can_be_merged() was awaited, not that
PullRequestHandler was constructed with the correct OwnersFileHandler instance;
update the tests around the GithubWebhook.process() calls (where
OwnersFileHandler.initialize is patched and mock_pr_handler is used) to also
assert that mock_pr_handler.assert_called_once_with(github_webhook=webhook,
owners_file_handler=mock_owners_instance) (or equivalent) after awaiting
webhook.process(), ensuring PullRequestHandler was instantiated with the actual
OwnersFileHandler instance returned by OwnersFileHandler.initialize; apply the
same assertion in both the shared recheck test and the review-thread test that
cover _recheck_merge_eligibility().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bdc78b8-efe2-47b5-adcc-cdb4b3389700
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
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
3 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
pull_request_review_threadevent handler ingithub_api.pycan-be-mergedonresolvedandunresolvedactions (always runs full check)created) before API bootstrap to save rate limitrequired_conversation_resolutionis disabledcheck_if_can_be_mergedevaluates ALL conditions (approvals, OWNERS, labels, checks, conversations) on every callstatusevent: clone + initialize for full merge check, hoist pending skip before bootstrap_recheck_merge_eligibility()shared helper for status + review_thread handlerspull_request_review_threadandstatusto example configsProblem
When review threads are resolved,
can-be-mergedwas not re-evaluated. PRs stayed in stale "unresolved conversations" state until another event triggered re-evaluation.Changes
webhook_server/libs/github_api.pypull_request_review_thread+statushandlers with clone + full merge check, hoist early skips before bootstrap, extract_recheck_merge_eligibility()webhook_server/utils/webhook.pyset()webhook_server/tests/test_github_api.pywebhook_server/tests/test_webhook.pyexamples/config.yamlpull_request_review_threadandstatuseventsexamples/.github-webhook-server.yamlpull_request_review_threadandstatuseventswebhook_server/tests/manifests/config.yamlpull_request_review_threadandstatuseventsTest plan
Closes #1038