fix: missing/queued required checks didn't block PR merge#928
Conversation
Fixed bug where PRs could merge when required checks were missing or queued. Added caching for all_required_status_checks() to reduce overhead. Fixed blocking PyGithub property accesses by wrapping in asyncio.to_thread. Added 9 comprehensive tests for missing/queued check scenarios. All 66 tests pass with 91% coverage maintained.
Fixed slow tests by mocking asyncio.wait() to avoid 30s timeouts: - test_get_logs_page: 30.12s → <0.1s - test_mcp_logging_configuration: 30.00s → <0.1s Full test suite runtime: ~81s → ~21s (74% faster)
WalkthroughAdds commit-status-aware logic and caching to CheckRunHandler, parallelizes last-commit check_run/status fetches, updates method signatures and call sites to pass commit statuses, and expands async tests plus test-time asyncio.wait mocks to avoid shutdown delays. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
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. |
|
/check-can-merge |
1 similar comment
|
/check-can-merge |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webhook_server/tests/test_logging_separation.py (1)
39-45: Consider extracting the mock_wait utility to reduce duplication.The
mock_waitfunction is duplicated in bothtest_logging_separation.py(lines 39-45) andtest_log_api.py(lines 644-650). Consider extracting this to a shared test utility module to follow DRY principles.You could create a shared utility in
webhook_server/tests/conftest.py:async def mock_asyncio_wait(tasks, timeout=None, return_when=None): """Mock asyncio.wait to prevent blocking during test shutdown.""" for task in tasks: task.cancel() return set(), tasksThen import and use it in both test files:
from webhook_server.tests.conftest import mock_asyncio_wait with patch("webhook_server.app.asyncio.wait", side_effect=mock_asyncio_wait): # test code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/handlers/check_run_handler.py(5 hunks)webhook_server/tests/test_check_run_handler.py(1 hunks)webhook_server/tests/test_log_api.py(2 hunks)webhook_server/tests/test_logging_separation.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
🧬 Code graph analysis (4)
webhook_server/libs/handlers/check_run_handler.py (1)
webhook_server/tests/conftest.py (2)
pull_request(106-107)PullRequest(90-102)
webhook_server/tests/test_check_run_handler.py (1)
webhook_server/libs/handlers/check_run_handler.py (1)
required_check_failed_or_no_status(363-407)
webhook_server/tests/test_logging_separation.py (1)
webhook_server/tests/test_log_api.py (1)
mock_wait(645-650)
webhook_server/tests/test_log_api.py (2)
webhook_server/tests/test_logging_separation.py (1)
mock_wait(40-45)webhook_server/tests/test_app.py (2)
client(36-38)client(344-345)
🪛 Ruff (0.14.5)
webhook_server/tests/test_logging_separation.py
40-40: Unused function argument: timeout
(ARG001)
40-40: Unused function argument: return_when
(ARG001)
webhook_server/tests/test_log_api.py
645-645: Unused function argument: timeout
(ARG001)
645-645: Unused function argument: return_when
(ARG001)
🔇 Additional comments (17)
webhook_server/tests/test_logging_separation.py (1)
40-40: Static analysis false positive on unused parameters.The
timeoutandreturn_whenparameters are intentionally unused but required to match theasyncio.waitsignature for proper mocking. These parameters ensure the mock can be used as a drop-in replacement.webhook_server/tests/test_log_api.py (2)
645-645: Static analysis false positive on unused parameters.Same as in
test_logging_separation.py, these parameters are required to match theasyncio.waitsignature for proper mocking.
644-667: LGTM - Test utility prevents blocking during shutdown.The mock implementation correctly cancels all tasks and returns immediately, preventing the 30-second shutdown timeout mentioned in the PR objectives. The test assertions remain valid.
Note: This duplicates the mock from
test_logging_separation.py. See the refactoring suggestion in that file to centralize this utility.webhook_server/libs/handlers/check_run_handler.py (5)
41-41: LGTM - Cache attribute for required status checks.The new cache attribute prevents redundant computation of required status checks across multiple invocations within the same webhook request handler.
367-374: LGTM - Core bug fix: Detects missing required checks.This change addresses the PR's main objective by identifying required checks that haven't been created yet. The set difference correctly identifies checks in
all_required_status_checksbut not present inlast_commit_check_runs.This prevents PRs from being merged when required checks are missing or queued, as stated in the PR title: "fix: missing/queued required checks didn't block PR merge".
410-435: LGTM - Caching optimization with proper implementation.The caching logic correctly:
- Returns cached value if available (line 411-412)
- Computes the full list of required checks
- Caches the result before returning (line 434)
The cache lifetime is scoped to the CheckRunHandler instance, which is created per webhook request, so no cache invalidation is needed.
440-440: LGTM - Prevents blocking on property access.Using
asyncio.to_threadto wrap therepository.privateproperty access prevents blocking the event loop, consistent with the PR's goal of preventing blocking PyGithub property accesses.
454-456: LGTM - Async wrapper for nested property access.The lambda wrapper inside
asyncio.to_threadcorrectly handles the nested property accessbranch_protection.required_status_checks.contexts, maintaining consistency with the async pattern used throughout the file.webhook_server/tests/test_check_run_handler.py (9)
640-663: LGTM - Comprehensive test for missing required checks.This test validates the core bug fix: missing required checks (not yet created) are correctly detected and reported as "not started". The test correctly:
- Mocks a scenario where TOX_STR is required but missing
- Verifies VERIFIED_LABEL_STR (successful) doesn't appear in the error
- Confirms the error message includes "not started"
665-692: LGTM - Test validates queued checks are treated as not started.This test ensures that checks with
conclusion=None(queued/in-progress state) are correctly identified as "not started" in the error message, preventing premature PR merges.
694-720: LGTM - Test validates failed check detection.Ensures failed checks appear in the error message with "failed" status while successful checks are excluded.
722-748: LGTM - Test validates no errors when all checks succeed.Correctly verifies that when all required checks are successful, an empty string is returned (no blocking errors).
750-779: LGTM - Test validates non-required checks are ignored.Important edge case: ensures that missing checks that are not in the required list don't cause false positive errors.
781-807: LGTM - Test validates multiple missing checks detection.Ensures all missing required checks appear in the error message, not just the first one found.
809-850: LGTM - Comprehensive test for mixed check states.This test validates complex scenarios with checks in various states (missing, queued, failed, successful) to ensure the logic handles all combinations correctly.
852-880: LGTM - Test validates in-progress checks are excluded from failures.Correctly ensures that checks being rerun (in progress) aren't reported as failed, avoiding false positive errors during check reruns.
882-907: LGTM - Test validates can-be-merged check is always ignored.Important test ensuring the special
can-be-mergedcheck doesn't cause recursive blocking (it shouldn't block itself).
GitHub has two APIs for commit checks: - Check Runs API (modern) - used by webhook server - Statuses API (legacy) - used by external services like pre-commit.ci Previously only Check Runs were validated, causing external service checks (like "pre-commit.ci - pr") to show as "not started" even when they passed. Changes: - Fetch both check runs AND statuses concurrently (asyncio.gather) - Validate statuses alongside check runs in merge checks - Map status states (success/pending/failure) to check outcomes - Add last_commit_statuses parameter to validation methods API optimization: 2 calls executed concurrently = same latency as 1 call Fixes: PR #928 incorrectly reporting pre-commit.ci as "not started"
…oved Previous commit added get_statuses() call to check_if_can_be_merged() but test only mocked get_check_runs(). Added get_statuses mock returning empty list to match production code behavior.
|
/check-can-merge |
1 similar comment
|
/check-can-merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
webhook_server/tests/test_check_run_handler.py (1)
642-909: New tests give strong coverage for required-check edge cases; consider adding CommitStatus-focused casesThe new suite around
required_check_failed_or_no_status(missing, queued, failed, mixed states, in-progress exclusions, and can-be-merged ignoring) accurately encodes the intended behavior and locks in the regression fix for missing/queued checks not blocking merges. One possible follow‑up improvement is to add a couple of tests that exercise thelast_commit_statusespath directly (e.g., required checks represented only asCommitStatuswithstate="pending"or"failure"), to ensure the legacy status API behavior is equally protected.webhook_server/libs/handlers/check_run_handler.py (3)
364-435:required_check_failed_or_no_statussemantics correctly enforce missing/queued/failed required checksThe refactor to:
- derive
required_checksonce fromall_required_status_checks,- compute
missing_required_checksacross both check runs and legacy statuses,- treat queued /
conclusion=Nonecheck runs and absent required checks as “not started”,- and classify non‑success
CommitStatusstates (pending→ “not started”,failure/error→ failed),ensures that:
- missing required checks now block merge,
- queued check-runs and pending statuses also block merge,
- and failures from either API surface are captured.
Two small polish opportunities that don’t affect correctness:
- Avoid repeated
await self.all_required_status_checks(...)inside the loop by reusing the localrequired_checksset.- When
exclude_in_progressends up empty, consider skipping the"Some check runs failed: ..."line altogether to avoid an empty “failed” message when everything is currently being rerun and is already reported via the in‑progress path.Functionally this looks solid for the regression you’re fixing.
437-487: Caching of required status checks and branch protection contexts looks correct for per-PR handler usageThe introduction of
_all_required_status_checksplus the existing_repository_private/_branch_required_status_checkscaches neatly avoids repeated GitHub API calls:
all_required_status_checksnow short‑circuits once computed and logs the combined list.get_branch_required_status_checksdefers all PyGithub property access (repository.private,get_branch,get_protection,.required_status_checks.contexts) intoasyncio.to_thread, keeping the event loop non‑blocking.- Private repositories still skip branch protection lookup early, as before.
Assuming
CheckRunHandlerinstances are scoped per webhook/PR (which matches how it’s constructed inPullRequestHandler), this caching is safe and beneficial. If you ever reuse a handler across PRs with different base refs, you may want an explicit invalidation strategy keyed bypull_request.base.ref, but that’s not required for the current design.
489-513: Including pendingCommitStatusentries inrequired_check_in_progresscloses the queued-check gapExtending
required_check_in_progressto:
- take
last_commit_statusesalongside check runs, and- add any required contexts with
state == "pending"tocheck_runs_in_progress,means queued/pending required checks reported only via the legacy status API now correctly block merge, aligned with the new tests. As above, you could micro‑optimize by reusing a cached
required_checksset instead of awaitingall_required_status_checksinside the comprehensions, but behavior is correct as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/handlers/check_run_handler.py(7 hunks)webhook_server/libs/handlers/pull_request_handler.py(3 hunks)webhook_server/tests/test_check_run_handler.py(3 hunks)webhook_server/tests/test_pull_request_handler.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
🧬 Code graph analysis (4)
webhook_server/tests/test_pull_request_handler.py (2)
webhook_server/tests/conftest.py (1)
github_webhook(111-131)webhook_server/tests/test_check_run_handler.py (2)
get_check_runs(517-518)get_check_runs(531-532)
webhook_server/tests/test_check_run_handler.py (1)
webhook_server/libs/handlers/check_run_handler.py (1)
required_check_failed_or_no_status(364-435)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/tests/conftest.py (2)
github_webhook(111-131)pull_request(106-107)webhook_server/tests/test_check_run_handler.py (2)
get_check_runs(517-518)get_check_runs(531-532)
webhook_server/libs/handlers/check_run_handler.py (1)
webhook_server/tests/conftest.py (2)
pull_request(106-107)PullRequest(90-102)
🔇 Additional comments (5)
webhook_server/tests/test_check_run_handler.py (2)
553-559: Updated call torequired_check_failed_or_no_statusmatches new signaturePassing
last_commit_statuses=[]andcheck_runs_in_progress=[]keeps this test aligned with the new API while still verifying the original failure behavior; no issues here.
611-641: Addition oflast_commit_statusesarg inrequired_check_in_progresstests is consistentExtending the call to pass an empty
last_commit_statuseslist keeps the test focused on check-runs-only behavior and matches the new handler signature.webhook_server/tests/test_pull_request_handler.py (1)
667-705:last_commitmock now correctly satisfies parallel check-runs/statuses fetchUpdating
test_check_if_can_be_merged_approvedto patchgithub_webhook.last_commitwithget_check_runsandget_statusesreturning empty lists matches the newasyncio.gather(... get_check_runs(), ... get_statuses())behavior incheck_if_can_be_merged. This keeps the test focused on the approval/labeling path without introducing network calls or unrelated failures.webhook_server/libs/handlers/pull_request_handler.py (2)
956-965: Parallelizing last-commit check-runs and statuses fetch is a good non-blocking improvementUsing:
_check_runs, _statuses = await asyncio.gather( asyncio.to_thread(lambda: list(self.github_webhook.last_commit.get_check_runs())), asyncio.to_thread(lambda: list(self.github_webhook.last_commit.get_statuses())), )reduces two blocking GitHub API calls to one concurrent operation while keeping them off the event loop. Any exceptions bubble into the surrounding
try/exceptwhere the merge check is already handled as a failure, so this remains safe from a correctness perspective.
975-999: Wiringlast_commit_statusesinto required-check evaluation correctly closes the merge gapPassing both
last_commit_check_runsandlast_commit_statusesinto:
required_check_in_progress(... last_commit_statuses=last_commit_statuses), andrequired_check_failed_or_no_status(... last_commit_statuses=last_commit_statuses, check_runs_in_progress=check_runs_in_progress),ensures merge eligibility now considers:
- in-progress/pending required checks (from either API),
- missing required checks,
- and failed required checks,
before deciding whether to add the
CAN_BE_MERGED_STRlabel or fail the merge check. This directly addresses the bug where missing/queued required checks previously allowed merges.
|
/check-can-merge |
5 similar comments
|
/check-can-merge |
|
/check-can-merge |
|
/check-can-merge |
|
/check-can-merge |
|
/check-can-merge |
GitHub Status API returns all historical statuses for a commit, not just the latest update. This caused checks like 'pre-commit.ci - pr' to show as "not started" even when the latest status was "success". Changes: - Filter statuses by highest ID per context (most recent) - Remove last_commit_statuses from required_check_in_progress (Status API has no in_progress state) - Clean up excessive debug logging (removed 13 verbose logs, kept 4 useful ones) - Add comprehensive test validating status+check run combined validation - Add debug logging for status fetching in pull_request_handler Fixes: Status checks showing incorrect state
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webhook_server/libs/handlers/check_run_handler.py(7 hunks)webhook_server/libs/handlers/pull_request_handler.py(3 hunks)webhook_server/tests/test_check_run_handler.py(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/handlers/check_run_handler.py
🧬 Code graph analysis (3)
webhook_server/tests/test_check_run_handler.py (4)
webhook_server/tests/test_pull_request_handler.py (1)
mock_pull_request(135-155)webhook_server/tests/test_runner_handler.py (1)
mock_pull_request(53-63)webhook_server/libs/handlers/check_run_handler.py (1)
required_check_failed_or_no_status(364-459)webhook_server/tests/conftest.py (1)
pull_request(106-107)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/tests/conftest.py (2)
github_webhook(111-131)pull_request(106-107)webhook_server/tests/test_check_run_handler.py (2)
get_check_runs(524-525)get_check_runs(538-539)
webhook_server/libs/handlers/check_run_handler.py (1)
webhook_server/tests/conftest.py (1)
PullRequest(90-102)
🔇 Additional comments (11)
webhook_server/tests/test_check_run_handler.py (2)
561-563: LGTM!The test correctly updates the call signature to match the new method signature with
last_commit_statusesandcheck_runs_in_progressparameters.
649-917: Excellent test coverage for required check validation.These new tests comprehensively cover various scenarios for required check validation including missing, queued, failed, and successful checks. The test suite properly validates edge cases like in-progress checks being excluded from failures and the can-be-merged check being ignored.
webhook_server/libs/handlers/check_run_handler.py (7)
42-42: LGTM!The cache field follows the established pattern used for
_branch_required_status_checksand will help reduce overhead from repeated status check calculations.
364-370: LGTM!The updated signature properly extends the method to handle commit statuses (legacy Statuses API) alongside check runs, addressing the bug described in the PR objectives.
373-419: LGTM!The status processing logic correctly handles the legacy Statuses API alongside the Check Runs API. The deduplication by highest
status.idis the correct approach for handling multiple status updates for the same context, and the logic properly maps status states (pending,failure,error) to the corresponding check outcomes.
420-440: LGTM!The updated check run processing correctly handles the case where the same check exists as both a CheckRun and a CommitStatus by prioritizing successful statuses, preventing duplicate error reporting.
461-487: LGTM!The caching logic is well-implemented and will reduce overhead from repeated calculations of required status checks during PR processing.
492-492: LGTM!Wrapping PyGithub property accesses in
asyncio.to_threadis the correct approach to prevent blocking the event loop during synchronous I/O operations. These changes align with the PR objectives to avoid blocking calls.Also applies to: 506-508
513-530: LGTM!The updated signature and added comment correctly clarify that only Check Runs (not Statuses) have an "in_progress" state, which is an important distinction between the two GitHub APIs.
webhook_server/libs/handlers/pull_request_handler.py (2)
959-973: LGTM!The parallel fetching of check runs and statuses using
asyncio.gatheris an excellent optimization that reduces latency by performing two API calls concurrently. The logging provides good observability for debugging.
984-987: LGTM!The updated method calls correctly pass the new parameters to align with the modified signatures in
check_run_handler.py. The distinction between which methods receivelast_commit_statusesis appropriate based on the functionality of each method.Also applies to: 997-1002
Address CodeRabbit AI review comments: - Add base.ref attribute to mock_pull_request fixture to prevent potential test failures - Add .id attributes to CommitStatus mocks for proper status deduplication
|
/reprocess |
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webhook_server/tests/test_check_run_handler.py (2)
54-59: Mock PullRequest fixture now correctly modelsbase.refand is used where neededSetting
mock_pr.base.ref = "main"aligns this fixture with how branch-required checks access PR data and addresses the earlier risk ofAttributeErrorwhenpull_request.base.refis used. Using this fixture explicitly intest_required_check_failed_or_no_status_with_commit_statuseskeeps that test realistic without over-configuring unrelated attributes. You could optionally migrate some ad‑hocMock()PRs in this file to reuse this fixture, but that's not required.Also applies to: 921-923
651-919: New required-check tests comprehensively cover missing/queued/failed/in‑progress/ignored scenariosThe added tests around
required_check_failed_or_no_statusexercise the key branches:
- missing vs queued vs failed required checks,
- all-success and non-required checks,
- multiple missing checks, mixed states,
- in-progress reruns excluded, and CAN_BE_MERGED_STR ignored even on failure.
These assertions line up with the documented behavior of the handler and should guard well against regressions in merge gating logic. If you want to trim repetition later, you could factor shared setup (e.g., constructing required lists and simple mocks) into small helpers or a
pytest.mark.parametrizetable, but the current explicit style is perfectly fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_check_run_handler.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
🧬 Code graph analysis (1)
webhook_server/tests/test_check_run_handler.py (4)
webhook_server/tests/test_pull_request_handler.py (1)
mock_pull_request(135-155)webhook_server/tests/test_runner_handler.py (1)
mock_pull_request(53-63)webhook_server/tests/test_owners_files_handler.py (1)
mock_pull_request(24-29)webhook_server/libs/handlers/check_run_handler.py (1)
required_check_failed_or_no_status(364-459)
🔇 Additional comments (2)
webhook_server/tests/test_check_run_handler.py (2)
563-565: Updatedrequired_check_failed_or_no_statuscall matches the new async signaturePassing
last_commit_check_runs,last_commit_statuses, andcheck_runs_in_progressexplicitly keeps this test in sync with the handler’s expanded parameters while still focusing only on the failed required-check path. This looks correct and future-proof.
921-979: Commit-status integration test accurately captures the PR‑928 regression scenarioThe
test_required_check_failed_or_no_status_with_commit_statusescase does a good job simulating PR‑928: it mixes check runs with legacy commit statuses, marks only the pre‑commit status as required, ensures.idis present for deduplication, and verifies that:
- the required status-based check (
pre-commit.ci - pr) is treated as successful,- only the queued required check-run (
VERIFIED_LABEL_STR) is reported as “not started”, and- non-required statuses like
CodeRabbitare ignored.This directly validates the intended behavior for missing/queued required checks when some checks only exist as commit statuses.
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Fixed bug where PRs could merge when required checks were missing or queued.
Added caching for all_required_status_checks() to reduce overhead.
Fixed blocking PyGithub property accesses by wrapping in asyncio.to_thread.
Added 9 comprehensive tests for missing/queued check scenarios.
All 66 tests pass with 91% coverage maintained.
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.