feat: Allow opt-in to auto re-run GH check runs on open PRs after merge to base branch #959
Conversation
…dated Implements automatic re-triggering of GitHub check suites when a PR's base branch receives new commits, helping keep CI results current and preventing merges of stale PRs with outdated test results. Key changes: - Add 'retrigger-checks-on-base-push' config option (default: true) in schema.yaml - Implement clean separation of concerns in pull_request_handler.py: * label_pull_request_by_merge_state() - ONLY handles PR labeling * _retrigger_check_suites_for_pr() - handles check suite re-requesting * retrigger_checks_for_out_of_date_prs_on_push() - coordinates labeling and check triggering - Update push_handler.py to delegate PR processing to PullRequestHandler - Add retrigger_checks_on_base_push attribute to GithubWebhook in github_api.py - Add comprehensive tests verifying the separated design and behavior The implementation checks each open PR targeting the updated branch and re-triggers check suites only for PRs with merge_state="behind", ensuring CI validation against the latest base branch commits.
…re-trigger-pr-not-rebased
WalkthroughAdds a config-driven opt-in to re-trigger configured CI checks for open pull requests when their base branch is pushed. New config keys, a GithubWebhook attribute, a RunnerHandler entrypoint, and pull-request labeling flow changes implement conditional retriggering. Changes
Sequence DiagramsequenceDiagram
participant Webhook as Webhook Receiver
participant PRH as PullRequestHandler
participant GHA as GitHub API
participant RH as RunnerHandler
participant CI as CI/Check Suites
rect rgb(200,220,255)
note over Webhook,PRH: Base-branch push event
Webhook->>PRH: push event
end
PRH->>GHA: list open PRs
GHA-->>PRH: open PRs
rect rgb(255,240,200)
loop for each open PR
PRH->>GHA: fetch PR details (mergeable_state)
GHA-->>PRH: mergeable_state
PRH->>PRH: label PR by merge state
end
end
rect rgb(220,255,220)
alt retrigger enabled && PR behind/blocked
PRH->>RH: run_retests(configured_tests, pull_request)
RH->>RH: map test names -> runner methods
par Concurrent retests
RH->>CI: spawn retest task 1
RH->>CI: spawn retest task 2
RH->>CI: spawn retest task N
end
CI-->>RH: results / errors
RH-->>PRH: completion / logs
else retrigger disabled or not applicable
PRH->>PRH: skip retrigger
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)webhook_server/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
webhook_server/**/*.{py,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
webhook_server/tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (11)📚 Learning: 2024-10-29T10:42:50.163ZApplied to files:
📚 Learning: 2024-10-29T08:09:57.157ZApplied to files:
📚 Learning: 2025-11-25T12:39:43.079ZApplied to files:
📚 Learning: 2024-10-08T09:19:56.185ZApplied to files:
📚 Learning: 2025-11-25T12:39:43.079ZApplied to files:
📚 Learning: 2025-11-25T12:39:43.079ZApplied to files:
📚 Learning: 2025-05-13T12:06:27.297ZApplied to files:
📚 Learning: 2025-11-25T12:39:43.079ZApplied to files:
📚 Learning: 2025-11-25T12:39:43.079ZApplied to files:
📚 Learning: 2025-10-28T13:04:00.466ZApplied to files:
📚 Learning: 2025-12-29T13:09:56.231ZApplied to files:
🪛 Ruff (0.14.10)webhook_server/tests/test_pull_request_handler.py32-32: Dynamically typed expressions (typing.Any) are disallowed in (ANN401) 32-32: Dynamically typed expressions (typing.Any) are disallowed in (ANN401) 472-472: Wrong type passed to first argument of Use a (PT006) 487-487: Boolean-typed positional argument in function definition (FBT001) 🔇 Additional comments (1)
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. |
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-959 published |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_pull_request_handler.py
🧰 Additional context used
📓 Path-based instructions (6)
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/**/*.py: Internal methods inwebhook_server/libs/can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/pull_request_handler.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/pull_request_handler.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Architecture guarantee:repository_datais ALWAYS set before handlers instantiate inGithubWebhook.process()with fail-fast exceptions - no defensive checks needed
Useawait asyncio.gather(*tasks, return_exceptions=True)for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Use structured logging with contextual parameters viaget_logger_with_params()including repository, hook_id, and component-specific identifiers for webhook correlation
Configuration access viaConfig(repository='org/repo-name')- repository parameter is required for per-repository overrides via.github-webhook-server.yaml
Handler pattern: implement__init__(self, github_webhook: GithubWebhook, ...)andprocess_event(event_data: dict) -> Nonefor all GitHub event handlers
Useself.github_webhook.unified_apifor all GitHub API operations in handlers - never access GitHub API directly
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.py
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration schema must be defined in
webhook_server/config/schema.yamlwith validation logic inwebhook_server/libs/config.py
Files:
webhook_server/config/schema.yaml
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_pull_request_handler.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/*check_run*.py : Repository cloning for check_run webhook events MUST be optimized by skipping clone when action != 'completed' or when check name is 'can-be-merged' with non-success conclusion
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to {config.yaml,.github-webhook-server.yaml} : Maintain backward compatibility ONLY for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration paths
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Configuration access via `Config(repository='org/repo-name')` - repository parameter is required for per-repository overrides via `.github-webhook-server.yaml`
Applied to files:
webhook_server/config/schema.yamlwebhook_server/libs/github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema must be defined in `webhook_server/config/schema.yaml` with validation logic in `webhook_server/libs/config.py`
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2025-02-25T12:01:42.999Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 666
File: webhook_server_container/libs/github_api.py:2254-2255
Timestamp: 2025-02-25T12:01:42.999Z
Learning: In the ProcessGithubWehook class, the `self.conventional_title` attribute is set during initialization by the `_repo_data_from_config()` method, and doesn't need additional existence checks.
Applied to files:
webhook_server/libs/github_api.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/github_api.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Architecture guarantee: `repository_data` is ALWAYS set before handlers instantiate in `GithubWebhook.process()` with fail-fast exceptions - no defensive checks needed
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-09T09:16:45.452Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-09T09:16:45.452Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/libs/handlers/issue_comment_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/tests/test_pull_request_handler.pywebhook_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/tests/test_pull_request_handler.pywebhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
webhook_server/tests/test_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_pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/*check_run*.py : Repository cloning for check_run webhook events MUST be optimized by skipping clone when action != 'completed' or when check name is 'can-be-merged' with non-success conclusion
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
🧬 Code graph analysis (4)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/libs/handlers/issue_comment_handler.py (2)
webhook_server/libs/handlers/runner_handler.py (1)
run_retests(747-776)webhook_server/tests/conftest.py (1)
pull_request(106-107)
webhook_server/tests/test_pull_request_handler.py (2)
webhook_server/libs/handlers/pull_request_handler.py (3)
label_and_rerun_checks_all_opened_pull_requests_merge_state_after_merged(410-437)label_pull_request_by_merge_state(880-901)_retrigger_check_suites_for_pr(903-925)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(747-776)
webhook_server/libs/handlers/pull_request_handler.py (4)
webhook_server/tests/conftest.py (3)
pull_request(106-107)github_webhook(111-131)PullRequest(90-102)webhook_server/utils/helpers.py (1)
format_task_fields(135-154)webhook_server/tests/test_runner_handler.py (1)
runner_handler(48-50)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(747-776)
🪛 Ruff (0.14.10)
webhook_server/libs/handlers/runner_handler.py
755-755: Logging statement uses f-string
(G004)
769-769: Logging statement uses f-string
(G004)
776-776: Logging statement uses f-string
(G004)
webhook_server/tests/test_pull_request_handler.py
441-441: Missing return type annotation for private function mock_label_side_effect
(ANN202)
463-463: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
478-478: Boolean-typed positional argument in function definition
(FBT001)
webhook_server/libs/handlers/pull_request_handler.py
887-887: Logging statement uses f-string
(G004)
919-919: Logging statement uses f-string
(G004)
922-922: Logging statement uses f-string
(G004)
🔇 Additional comments (3)
webhook_server/libs/github_api.py (1)
711-714: LOW: Newretrigger_checks_on_base_pushattribute is correctly sourced and typedUsing
Config.get_value("retrigger-checks-on-base-push", return_on_none=False, extra_dict=repository_config)respects the expected precedence: local.github-webhook-server.yaml→ repo-specific config → root config → defaultFalse. The explicitboolannotation and wiring intoGithubWebhookmatch how other feature flags are handled, and it lines up with the schema addition.No changes needed here.
As per coding guidelines, this follows the config/schema pattern without extra defensive checks.
webhook_server/libs/handlers/issue_comment_handler.py (1)
5-6: LOW: Retest handling nicely centralized viaRunnerHandler.run_retestsTyping
tasksaslist[Coroutine[Any, Any, Any] | Task[Any]]and delegating/retestexecution torunner_handler.run_retests(...)removes duplicate mapping/gather logic from this handler and keeps all CI execution paths inRunnerHandler. This also alignsissue_comment-initiated retests with the new “retrigger on base push” flow.No functional issues spotted here.
Also applies to: 106-137, 449-451
webhook_server/libs/handlers/pull_request_handler.py (1)
167-173: LOW: Merge‑state labeling and retrigger flow are well factored and align with the new config flagThe new flow:
- On
closed+merged,process_pull_request_webhook_datacalls
label_and_rerun_checks_all_opened_pull_requests_merge_state_after_merged().- That method:
- Waits 30s (non‑blocking
asyncio.sleep) to let GitHub computemergeable_state.- Fetches open PRs via
await asyncio.to_thread(self.repository.get_pulls, state="open").- Calls
label_pull_request_by_merge_statefor each PR and uses its returnedmerge_state.- If
github_webhook.retrigger_checks_on_base_pushisTrueand the state is"behind"or"blocked", delegates to_retrigger_check_suites_for_pr.
label_pull_request_by_merge_stateis now a pure “labeling + state” helper: it wraps themergeable_stateproperty inasyncio.to_thread, updates only theNEEDS_REBASE/HAS_CONFLICTSlabels, and returns the state string (early‑returning on"unknown"). Tests also assert that this method does not itself trigger checks, keeping separation of concerns clear.
_retrigger_check_suites_for_prcorrectly:
- Logs via
logger.stepwith structured task fields.- Uses
github_webhook.current_pull_request_supported_retestas the single source of truth for which checks can run.- No‑ops (with a debug log) when no checks are configured.
- Delegates execution to
runner_handler.run_retests, reusing the shared mapping and error handling there.Overall, this matches the intended “opt‑in retrigger after merge” behavior and respects the async + PyGithub access guidelines.
Also applies to: 410-438, 880-902, 903-925
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
webhook_server/libs/handlers/pull_request_handler.py
🧰 Additional context used
📓 Path-based instructions (4)
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/**/*.py: Internal methods inwebhook_server/libs/can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Files:
webhook_server/libs/handlers/pull_request_handler.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/libs/handlers/pull_request_handler.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/libs/handlers/pull_request_handler.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Architecture guarantee:repository_datais ALWAYS set before handlers instantiate inGithubWebhook.process()with fail-fast exceptions - no defensive checks needed
Useawait asyncio.gather(*tasks, return_exceptions=True)for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Use structured logging with contextual parameters viaget_logger_with_params()including repository, hook_id, and component-specific identifiers for webhook correlation
Configuration access viaConfig(repository='org/repo-name')- repository parameter is required for per-repository overrides via.github-webhook-server.yaml
Handler pattern: implement__init__(self, github_webhook: GithubWebhook, ...)andprocess_event(event_data: dict) -> Nonefor all GitHub event handlers
Useself.github_webhook.unified_apifor all GitHub API operations in handlers - never access GitHub API directly
Files:
webhook_server/libs/handlers/pull_request_handler.py
🧠 Learnings (5)
📚 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-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-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: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/*check_run*.py : Repository cloning for check_run webhook events MUST be optimized by skipping clone when action != 'completed' or when check name is 'can-be-merged' with non-success conclusion
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
🧬 Code graph analysis (1)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/utils/helpers.py (1)
format_task_fields(135-154)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(747-776)
🪛 Ruff (0.14.10)
webhook_server/libs/handlers/pull_request_handler.py
887-887: Logging statement uses f-string
(G004)
915-915: Logging statement uses f-string
(G004)
918-918: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: conventional-title
- GitHub Check: build-container
- GitHub Check: python-module-install
- GitHub Check: pre-commit
- GitHub Check: tox
🔇 Additional comments (3)
webhook_server/libs/handlers/pull_request_handler.py (3)
880-901: LGTM! Clean return type enhancement.Changing the return type from
Nonetostris backward compatible—existing callers (like line 743) can ignore the return value. The new return enables the caller at line 432 to act onmerge_statewithout re-querying PyGithub, reducing API calls.
903-922: LGTM! Clean delegation to existing retest infrastructure.The implementation correctly:
- Wraps
pull_request.numberproperty access inasyncio.to_thread()per guidelines- Uses structured logging with
format_task_fields()for task correlation- Delegates to
runner_handler.run_retests()(verified in relevant code snippets)- Returns early when no checks are configured (defensive but acceptable)
410-437: LGTM! Config attributes verified and implementation confirmed.Both config attributes exist and are properly typed in the
GithubWebhookclass:
retrigger_checks_on_base_push: bool— opt-in flag initialized from configcurrent_pull_request_supported_retest: list[str]— populated from repository settingsThe
_retrigger_check_suites_for_pr()method correctly follows async patterns: wraps PyGithub property access withasyncio.to_thread(), checks for available checks before processing, and delegates torunner_handler.run_retests(). The merge state filtering (only "behind" and "blocked") is tested across multiple scenarios and prevents unnecessary retriggers for clean or conflicted PRs.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/config/schema.yamlwebhook_server/tests/test_pull_request_handler.py
🧰 Additional context used
📓 Path-based instructions (4)
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration schema must be defined in
webhook_server/config/schema.yamlwith validation logic inwebhook_server/libs/config.py
Files:
webhook_server/config/schema.yaml
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/tests/test_pull_request_handler.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/tests/test_pull_request_handler.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_pull_request_handler.py
🧠 Learnings (12)
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to {config.yaml,.github-webhook-server.yaml} : Maintain backward compatibility ONLY for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration paths
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema must be defined in `webhook_server/config/schema.yaml` with validation logic in `webhook_server/libs/config.py`
Applied to files:
webhook_server/config/schema.yaml
📚 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/config/schema.yamlwebhook_server/tests/test_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/tests/test_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/tests/test_pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
webhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
webhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub method calls MUST be wrapped with `asyncio.to_thread()` to avoid blocking the FastAPI event loop - including `.get_*()`, `.create_*()`, `.edit()`, `.add_to_*()`, `.remove_from_*()`
Applied to files:
webhook_server/tests/test_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_pull_request_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : PyGithub PaginatedList iteration MUST be wrapped in `asyncio.to_thread()` with list conversion - never iterate directly to avoid blocking the event loop
Applied to files:
webhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/tests/test_pull_request_handler.py
🧬 Code graph analysis (1)
webhook_server/tests/test_pull_request_handler.py (2)
webhook_server/libs/handlers/pull_request_handler.py (3)
label_and_rerun_checks_all_opened_pull_requests_merge_state_after_merged(410-437)label_pull_request_by_merge_state(880-901)_retrigger_check_suites_for_pr(903-921)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(747-776)
🪛 Ruff (0.14.10)
webhook_server/tests/test_pull_request_handler.py
30-30: Missing return type annotation for private function _sync_to_thread
(ANN202)
30-30: Missing type annotation for *args
(ANN002)
30-30: Missing type annotation for **kwargs
(ANN003)
470-470: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
485-485: Boolean-typed positional argument in function definition
(FBT001)
🔇 Additional comments (7)
webhook_server/config/schema.yaml (2)
86-89: LGTM: Global config property correctly defined.The new
retrigger-checks-on-base-pushglobal property is well-structured:
- Type and default value follow the established pattern
- Description accurately reflects that re-triggering occurs when a PR is merged (not on arbitrary pushes)
- Optional property maintains backward compatibility
Based on learnings, this maintains backward compatibility for user-facing configuration.
306-309: LGTM: Per-repository override correctly defined.The per-repository
retrigger-checks-on-base-pushproperty correctly mirrors the global property, allowing repository-specific configuration. This follows the established pattern for configuration overrides.webhook_server/tests/test_pull_request_handler.py (5)
291-292: LGTM: Method rename reflects expanded functionality.The method name change from
label_all_opened_pull_requests_merge_state_after_mergedtolabel_and_rerun_checks_all_opened_pull_requests_merge_state_after_mergedaccurately describes the new behavior (labeling + optional check retriggering). All test references are updated consistently.Also applies to: 429-431
413-432: LGTM: Test correctly verifies default behavior without retriggering.The test properly verifies that when
retrigger_checks_on_base_pushis disabled (line 424), only labeling occurs without check retriggering. The test correctly patchesasyncio.sleepto avoid real delays.Minor note: This test doesn't patch
asyncio.to_thread(unlike newer tests below), relying on the real implementation. While this works because the mock object is thread-safe, consider using the_sync_to_threadpattern for consistency with lines 460, 496, etc.
434-466: LGTM: Comprehensive test for retrigger behavior.The test correctly verifies that:
- Retriggering is conditional on
retrigger_checks_on_base_pushbeing enabled- Only PRs with
merge_state="behind"trigger checks- The
_sync_to_threadpattern is properly used to avoid blocking operationsThe
mock_label_side_effecthelper now includes proper type annotations, addressing the previous review feedback.
469-513: LGTM: Excellent parametrized test coverage for merge states.The parametrized test comprehensively verifies retrigger behavior across all GitHub merge states:
- Correctly identifies
behindandblockedas retriggering states- Verifies non-retriggering states (
clean,dirty,unstable,unknown)- Properly uses the
_sync_to_threadpattern throughoutNote: The Ruff PT006 warning about tuple syntax is a false positive—the comma-separated string format is valid pytest parametrize syntax.
1895-1925: LGTM: Comprehensive tests for _retrigger_check_suites_for_pr.The first two tests provide excellent coverage:
test_retrigger_check_suites_for_pr_successverifies that configured checks are passed torun_reteststest_retrigger_check_suites_for_pr_no_check_suitesverifies early return when no checks are configuredBoth properly use the
_sync_to_threadpattern and verify the correct integration points.
rnetser
changed the title
|
/retest conventional-title |
myakove-bot
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
Added type annotations to _sync_to_thread shim function using modern Python type parameter syntax. Added missing asyncio.to_thread patch in test_retrigger_check_suites_for_pr_behind_merge to ensure consistent test behavior. Fixed test docstring to accurately reflect exception propagation behavior.
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-959 published |
Summary by CodeRabbit
New Features
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.