Skip to content

Commit 38422af

Browse files
authored
fix: re-evaluate can-be-merged when review threads are resolved (#1038) (#1039)
## Summary - Add `pull_request_review_thread` event handler in `github_api.py` - Re-evaluate `can-be-merged` on `resolved` and `unresolved` actions (always runs full check) - Skip non-actionable events (`created`) before API bootstrap to save rate limit - Skip when `required_conversation_resolution` is disabled - Clone repo and initialize OwnersFileHandler — `check_if_can_be_merged` evaluates ALL conditions (approvals, OWNERS, labels, checks, conversations) on every call - Add `status` event: clone + initialize for full merge check, hoist pending skip before bootstrap - Extract `_recheck_merge_eligibility()` shared helper for status + review_thread handlers - Update webhook if configured events differ from existing (smart event reconciliation) - Add `pull_request_review_thread` and `status` to example configs ## Problem When review threads are resolved, `can-be-merged` was not re-evaluated. PRs stayed in stale "unresolved conversations" state until another event triggered re-evaluation. ## Changes | File | Change | |---|---| | `webhook_server/libs/github_api.py` | Add `pull_request_review_thread` + `status` handlers with clone + full merge check, hoist early skips before bootstrap, extract `_recheck_merge_eligibility()` | | `webhook_server/utils/webhook.py` | Smart webhook event reconciliation — update existing hooks when events change, dedupe with `set()` | | `webhook_server/tests/test_github_api.py` | Parameterized tests for status states, review thread actions, bootstrap skip assertions | | `webhook_server/tests/test_webhook.py` | Parameterized webhook event-update tests | | `examples/config.yaml` | Add `pull_request_review_thread` and `status` events | | `examples/.github-webhook-server.yaml` | Add `pull_request_review_thread` and `status` events | | `webhook_server/tests/manifests/config.yaml` | Add `pull_request_review_thread` and `status` events | ## Test plan - [x] All 1434 tests pass, 90.37% coverage - [x] Deployed and verified: resolved review threads trigger can-be-merged re-evaluation - [x] Webhook events updated on GitHub repos automatically Closes #1038
1 parent d07a720 commit 38422af

7 files changed

Lines changed: 355 additions & 32 deletions

File tree

examples/.github-webhook-server.yaml

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@ pypi:
2121
events:
2222
- push
2323
- pull_request
24-
- issue_comment
25-
- check_run
2624
- pull_request_review
2725
- pull_request_review_comment
26+
- pull_request_review_thread
27+
- issue_comment
28+
- check_run
29+
- status
2830

2931
# Tox configuration
3032
tox:

examples/config.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,11 @@ repositories:
150150
events: # To listen to all events do not send events
151151
- push
152152
- pull_request
153+
- pull_request_review
154+
- pull_request_review_thread
153155
- issue_comment
154156
- check_run
155-
- pull_request_review
157+
- status
156158
tox:
157159
main: all # Run all tests in tox.ini when pull request parent branch is main
158160
dev: testenv1,testenv2 # Run testenv1 and testenv2 tests in tox.ini when pull request parent branch is dev

webhook_server/libs/github_api.py

Lines changed: 72 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,59 @@ def redact_output(value: str) -> str:
409409
self.ctx.fail_step("repo_clone", ex, traceback.format_exc())
410410
raise RuntimeError(f"Repository clone failed: {ex}") from ex
411411

412+
async def _recheck_merge_eligibility(self, pull_request: PullRequest) -> None:
413+
"""Clone repo and re-evaluate can-be-merged for the PR.
414+
415+
Clone required: check_if_can_be_merged evaluates ALL conditions
416+
(approvals, OWNERS, labels, checks, conversations) on every call,
417+
and OWNERS file processing requires a local checkout.
418+
"""
419+
await self._clone_repository(pull_request=pull_request)
420+
owners_file_handler = OwnersFileHandler(github_webhook=self)
421+
owners_file_handler = await owners_file_handler.initialize(pull_request=pull_request)
422+
await PullRequestHandler(github_webhook=self, owners_file_handler=owners_file_handler).check_if_can_be_merged(
423+
pull_request=pull_request
424+
)
425+
412426
async def process(self) -> Any:
427+
# Early exit for pull_request_review_thread events that don't need processing.
428+
# Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid
429+
# burning rate limit on get_user() calls for skipped events.
430+
if self.github_event == "pull_request_review_thread":
431+
action = self.hook_data["action"]
432+
if action not in ("resolved", "unresolved") or not self.required_conversation_resolution:
433+
if self.ctx:
434+
self.ctx.start_step("webhook_routing", event_type=self.github_event)
435+
skip_reason = (
436+
f"action={action}, skipped"
437+
if action not in ("resolved", "unresolved")
438+
else "required_conversation_resolution disabled"
439+
)
440+
self.logger.info(
441+
f"{self.log_prefix} "
442+
f"Webhook processing completed successfully: pull_request_review_thread "
443+
f"({skip_reason}) - no metrics collected",
444+
)
445+
await self._update_context_metrics()
446+
return None
447+
448+
# Early exit for status events with pending state — only terminal states
449+
# (success, failure, error) need processing.
450+
# Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid
451+
# burning rate limit on get_user() calls for skipped events.
452+
if self.github_event == "status":
453+
state = self.hook_data["state"]
454+
if state == "pending":
455+
if self.ctx:
456+
self.ctx.start_step("webhook_routing", event_type=self.github_event)
457+
self.logger.info(
458+
f"{self.log_prefix} "
459+
f"Webhook processing completed successfully: status "
460+
f"(state=pending, skipped) - no metrics collected",
461+
)
462+
await self._update_context_metrics()
463+
return None
464+
413465
# Initialize auto-verified users from API users (async operation)
414466
await self.add_api_users_to_auto_verified_and_merged_users()
415467

@@ -508,8 +560,9 @@ async def process(self) -> Any:
508560
self.last_committer = getattr(self.last_commit.committer, "login", self.parent_committer)
509561

510562
# Clone repository for local file processing (OWNERS, changed files)
511-
# For check_run and status events, cloning happens later only when needed
512-
if self.github_event not in ("check_run", "status"):
563+
# For check_run, status, and pull_request_review_thread events,
564+
# cloning happens later only when needed (inside their respective handlers)
565+
if self.github_event not in ("check_run", "status", "pull_request_review_thread"):
513566
await self._clone_repository(pull_request=pull_request)
514567

515568
if self.github_event == "issue_comment":
@@ -606,28 +659,15 @@ async def process(self) -> Any:
606659
return None
607660

608661
elif self.github_event == "status":
609-
# Skip pending state — only terminal states (success, failure, error) trigger re-evaluation
662+
# Pending state already filtered by early exit above — only terminal states reach here
610663
state = self.hook_data["state"]
611-
if state == "pending":
612-
token_metrics = await self._get_token_metrics()
613-
self.logger.info(
614-
f"{self.log_prefix} "
615-
f"Webhook processing completed successfully: status (state=pending, skipped) - "
616-
f"{token_metrics}",
617-
)
618-
await self._update_context_metrics()
619-
return None
620-
621664
context_name = self.hook_data["context"]
622665
self.logger.info(
623666
f"{self.log_prefix} Status check '{context_name}' reached terminal state ({state}), "
624667
f"re-evaluating can-be-merged"
625668
)
626669

627-
owners_file_handler = OwnersFileHandler(github_webhook=self)
628-
await PullRequestHandler(
629-
github_webhook=self, owners_file_handler=owners_file_handler
630-
).check_if_can_be_merged(pull_request=pull_request)
670+
await self._recheck_merge_eligibility(pull_request=pull_request)
631671

632672
token_metrics = await self._get_token_metrics()
633673
self.logger.info(
@@ -636,6 +676,21 @@ async def process(self) -> Any:
636676
await self._update_context_metrics()
637677
return None
638678

679+
elif self.github_event == "pull_request_review_thread":
680+
# Action already filtered above (only resolved/unresolved reach here)
681+
action = self.hook_data["action"]
682+
self.logger.info(f"{self.log_prefix} Review thread {action}, re-evaluating can-be-merged")
683+
684+
await self._recheck_merge_eligibility(pull_request=pull_request)
685+
686+
token_metrics = await self._get_token_metrics()
687+
self.logger.info(
688+
f"{self.log_prefix} Webhook processing completed successfully: "
689+
f"pull_request_review_thread - {token_metrics}",
690+
)
691+
await self._update_context_metrics()
692+
return None
693+
639694
else:
640695
# Log warning when no PR found
641696
self.logger.warning(

webhook_server/tests/manifests/config.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@ repositories:
3838
events: # To listen to all events do not send events
3939
- push
4040
- pull_request
41+
- pull_request_review
42+
- pull_request_review_thread
4143
- issue_comment
4244
- check_run
43-
- pull_request_review
45+
- status
4446
tox-python-version: "3.8"
4547
tox:
4648
main: all # Run all tests in tox.ini when pull request parent branch is main

0 commit comments

Comments
 (0)