fix: handle status events to trigger can-be-merged re-evaluation (#1034)#1036
Merged
fix: handle status events to trigger can-be-merged re-evaluation (#1034)#1036
Conversation
External services like pre-commit.ci report via GitHub's Status API. When a status check succeeds, re-evaluate can-be-merged for the PR.
|
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 (1)
WalkthroughImplements handling for GitHub Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub Webhook
participant Processor as github_api.process()
participant PRLookup as get_pull_request()
participant Repo as Repository Clone
participant Handler as PullRequestHandler
participant Metrics as Metrics Logger
GitHub->>Processor: status event (commit.sha, state)
alt state == "pending"
Processor->>Metrics: log skip / increment metric
Processor-->>GitHub: exit
else state in ("success","failure","error")
Processor->>PRLookup: find open PR(s) matching commit.sha
PRLookup-->>Processor: PR found / not found
alt PR found
Processor->>Repo: clone repo (deferred until needed)
Processor->>Handler: init OwnersFileHandler & run can-be-merged evaluation
Handler-->>Metrics: log evaluation results
Metrics-->>Processor: update context
else no PR found
Processor->>Metrics: log missing PR, exit
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
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. |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoHandle status events to trigger can-be-merged re-evaluation
WalkthroughsDescription• Handle GitHub Status API events to trigger can-be-merged re-evaluation • Skip processing for non-success status states (pending, failure, error) • Add SHA-based PR lookup fallback for status events • Add comprehensive test coverage for status event handling Diagramflowchart LR
A["Status Event Webhook"] -->|state=success| B["Clone Repository"]
A -->|state=pending/failure| C["Skip Processing"]
B --> D["Initialize OwnersFileHandler"]
D --> E["Re-evaluate can-be-merged"]
E --> F["Log Completion"]
C --> F
G["Status Event"] -->|SHA lookup| H["Find PR by head.sha"]
H --> I["Return PR"]
File Changes1. webhook_server/libs/github_api.py
|
Code Review by Qodo
1. get_pulls iteration not threaded
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_github_api.py`:
- Around line 998-1001: The test function test_get_pull_request_from_status_sha
declares unused fixture parameters minimal_hook_data and minimal_headers; remove
these parameters from the test signature so pytest won't instantiate those
fixtures unnecessarily, leaving only the fixtures actually used (e.g., logger)
and update any references inside the test if they mistakenly referenced the
removed names; ensure the pytest.mark.asyncio decorator and async signature
remain intact for the test coroutine.
🪄 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: 6f6e551b-a923-4f47-8459-67fba64015fa
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- Wrap PaginatedList iteration and .head.sha access in asyncio.to_thread for both check_run and status SHA lookups - Re-evaluate can-be-merged on failure/error states (terminal), skip only pending (non-terminal) - Remove unused test fixture parameters
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 610-611: The status-handling flow currently uses defensive .get()
on the webhook payload (e.g., state = self.hook_data.get("state", "")) which
hides malformed payloads; change these to direct accesses (e.g.,
self.hook_data["state"]) for required fields in the status flow so the code
fails fast (allow the KeyError to propagate or convert it to an explicit
exception with context), and apply the same change to the other .get() usages
against self.hook_data in that flow so missing GitHub fields are surfaced
immediately.
- Around line 627-633: The status re-evaluation path currently does heavyweight
work by awaiting self._clone_repository and initializing OwnersFileHandler
before calling PullRequestHandler.check_if_can_be_merged; remove these steps for
the status-only flow and invoke check_if_can_be_merged without performing a
local checkout or full OWNERS initialization. Concretely, stop calling
_clone_repository and OwnersFileHandler.initialize in this path, instead pass a
None or lightweight placeholder for owners_file_handler (or have
PullRequestHandler lazily load OWNERS only when needed) so
check_if_can_be_merged (method check_if_can_be_merged on PullRequestHandler)
runs using only PR/commit API state and cached/graphql queries.
- Around line 912-915: The loop that awaits asyncio.to_thread(_get_pr_head_sha,
...) for each PullRequest (open_pulls from self.repository.get_pulls) should be
batched with asyncio.gather to avoid sequential blocking: call asyncio.to_thread
once to get list(open_pulls), then create a comprehension of
asyncio.to_thread(_get_pr_head_sha, pr) for each pr and await asyncio.gather to
get head_shas, then iterate zip(open_pulls, head_shas) and compare pr_head_sha
to head_sha; keep direct access to cached properties like _pull_request.title
and _pull_request.number without wrapping them in to_thread.
In `@webhook_server/tests/test_github_api.py`:
- Around line 819-1004: Add a parallel test to cover the terminal "error" state:
create a new async test (e.g.,
test_process_status_event_error_triggers_reevaluation) that mirrors
test_process_status_event_failure_triggers_reevaluation but sets
status_data["state"]="error", constructs the same mocks (GithubWebhook,
get_api_with_highest_rate_limit, get_github_repo_api, PullRequestHandler, etc.),
patches webhook._clone_repository and OwnersFileHandler.initialize as in the
failure test, calls await webhook.process(), and asserts
PullRequestHandler.return_value.check_if_can_be_merged.assert_awaited_once() to
ensure parity for terminal-state handling.
🪄 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: d372d77a-5e67-49fa-89fc-fe28f8b64e32
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
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 921-934: For the status webhook handling, reorder the logic in the
method that checks self.github_event so the head-SHA filter runs before using
commit_obj.get_pulls(): when self.github_event == "status", first compute sha =
self.hook_data["sha"], fetch open_pulls via self.repository.get_pulls and
resolve each PR head SHA using _get_pr_head_sha (as in the existing head_shas /
zip loop) and return the matching PR if pr_head_sha == sha; only if no matching
open PR is found then fall back to calling commit_obj.get_pulls() (or whatever
existing commit-based lookup is currently used). This ensures the head-SHA match
logic (variables: sha, open_pulls, head_shas, _get_pr_head_sha) executes before
the commit_obj.get_pulls() fallback for status events.
In `@webhook_server/tests/test_github_api.py`:
- Around line 819-1045: Consolidate the four near-duplicate tests
(test_process_status_event_success, test_process_status_event_pending_skipped,
test_process_status_event_failure_triggers_reevaluation,
test_process_status_event_error_triggers_reevaluation) into a single
parameterized test using pytest.mark.parametrize that iterates over (state,
expected_call) pairs (e.g. ("success", True), ("pending", False), ("failure",
True), ("error", True)); move the common setup (logger, status_data template,
headers, tempfile patching, and all patched calls like Config,
get_api_with_highest_rate_limit, get_github_repo_api,
get_repository_github_app_api, get_apis_and_tokes_from_config, and
PullRequestHandler) into the test body or a helper function/fixture and only set
status_data["state"] from the param before instantiating GithubWebhook and
awaiting process(); finally assert
mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() when
expected_call is True and .assert_not_awaited() when False so the matrix drives
the behavior instead of copying scaffolding.
- Around line 1047-1085: The test test_get_pull_request_from_status_sha omits a
commit object so the code path in GithubWebhook.get_pull_request that falls back
to status- SHA lookup isn't exercised; add a "commit" key to status_data whose
mock commit.get_pulls() returns a PR whose head.sha is different (stale) to
simulate a mismatched commit PR, and ensure the patched
get_repository_github_app_api/get_github_repo_api behavior remains so that
mock_repo.get_pulls(state="open") returns the correct PR with head.sha
"status-sha-123"; this forces get_pull_request() to ignore the commit PR and
proceed to the status- SHA lookup and still assert result == mock_pr and
mock_repo.get_pulls.assert_called_once_with(state="open").
🪄 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: c452f2b8-692d-4dc4-8905-5599afdd4c23
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
- Move check_run/status SHA lookups before commit.get_pulls() fallback - Parameterize 4 status state tests into single test - Add commit object with stale PR to status SHA test
myakove-bot
added
size/L
and removed
commented-myakove
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
size/XL
labels
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 609-630: The code currently skips only "pending" but still re-runs
PullRequestHandler.check_if_can_be_merged for failure/error; change the
conditional to only proceed when self.hook_data["state"] == "success" and return
early for any other state (including "pending", "failure", "error"), logging the
skipped state and calling token metrics/_update_context_metrics as currently
done; locate the block around self.hook_data["state"], update the if-check to
require "success" before instantiating OwnersFileHandler and calling
PullRequestHandler(...).check_if_can_be_merged(pull_request=pull_request).
In `@webhook_server/tests/test_github_api.py`:
- Around line 919-925: Add an explicit assertion that the commit fallback was
not used by asserting mock_repo.get_commit was never called in the test where
the status SHA already matches an open PR; after setting up stale_pr,
mock_commit.get_pulls, and mock_repo.get_commit, add a check using
mock_repo.get_commit.assert_not_called() (and similarly in the other test case
mentioned) to ensure get_commit() is not invoked when an open PR's head.sha
matches the status.
🪄 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: 8a9abd67-ac4e-4a57-8347-7c58a210532f
📒 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 |
3 tasks
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
statusevent handler ingithub_api.pyso external services using GitHub's Status API (e.g.,pre-commit.ci) triggercan-be-mergedre-evaluation when their checks succeedstatusevents inget_pull_request()Problem
When
pre-commit.cicompletes, it reports via GitHub's Status API (not Check Runs API). The webhook server only handledcheck_runevents, socan-be-mergedwas never re-evaluated after pre-commit.ci finished. This caused permanent "Some check runs not started: pre-commit.ci - pr" failures.Changes
webhook_server/libs/github_api.pystatusevent handler inprocess(), skip clone for status events until needed, SHA-based PR lookup fallbackwebhook_server/tests/test_github_api.pyTest plan
Closes #1034
Summary by CodeRabbit
New Features
Tests