feat: support user-defined custom check runs #961
Conversation
Address code review findings to improve stability and safety: - Replace unsafe dictionary access with .get() method at 5 locations to prevent KeyError crashes when 'name' field is missing - Add empty-string validation for secret redaction to prevent whitespace in redaction list - Add COMPLETED_STR constant for check run status - Implement comprehensive command security validation - Add dedicated test coverage for command security and custom checks This ensures custom check runs fail gracefully with proper logging rather than crashing on malformed configuration.
|
/wip |
|
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. |
WalkthroughAdds repository-scoped custom check runs: schema entry; loading/validation in the GitHub webhook; new check-run status APIs; queuing and concurrent execution in PR worktrees; retest orchestration updated to include custom checks; extensive tests for validation and execution. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub API
participant PR as Pull Request Event
participant PRHandler as PullRequest Handler
participant CheckHandler as CheckRun Handler
participant Runner as Runner Handler
participant Worktree as Repo Worktree
participant Exec as Command Executor
GH->>PR: pull_request.opened / synchronize
PR->>PRHandler: process_opened_or_synchronize
rect rgb(230,245,255)
note over PRHandler,CheckHandler: Queuing phase
PRHandler->>CheckHandler: set_custom_check_queued(name)
CheckHandler->>GH: Report check run: queued
end
rect rgb(245,230,255)
note over PRHandler,Runner: Execution phase
PRHandler->>Runner: run_custom_check(pull_request, check_config)
Runner->>CheckHandler: set_custom_check_in_progress(name)
CheckHandler->>GH: Report check: in_progress
Runner->>Worktree: checkout PR branch (worktree)
Runner->>Exec: execute configured command (cwd=worktree, env=merged)
end
rect rgb(235,255,235)
note over Runner,CheckHandler: Result reporting
alt command succeeds
Runner->>CheckHandler: set_custom_check_success(name, output)
CheckHandler->>GH: Report check: success
else command fails
Runner->>CheckHandler: set_custom_check_failure(name, output)
CheckHandler->>GH: Report check: failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
myakove-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_command_security.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/utils/command_security.pywebhook_server/utils/constants.py
🧰 Additional context used
📓 Path-based instructions (7)
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/utils/constants.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/command_security.pywebhook_server/tests/test_command_security.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.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/utils/constants.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/command_security.pywebhook_server/tests/test_command_security.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.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_custom_check_runs.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_command_security.pywebhook_server/tests/test_pull_request_handler.py
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/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.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/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.py
webhook_server/libs/handlers/*check_run*.py
📄 CodeRabbit inference engine (CLAUDE.md)
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
Files:
webhook_server/libs/handlers/check_run_handler.py
🧠 Learnings (19)
📚 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/utils/constants.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/command_security.pywebhook_server/tests/test_command_security.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_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: 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/*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/config/schema.yamlwebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_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/tests/**/*.py : Test files MUST use `TEST_GITHUB_TOKEN = 'ghp_test1234...'` with `# pragma: allowlist secret` comment to avoid security warnings in test tokens
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_command_security.pywebhook_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 : Code coverage of 90% or higher is MANDATORY - measured via `pytest --cov=webhook_server` and enforced in CI
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_command_security.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.pywebhook_server/tests/test_check_run_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/libs/handlers/issue_comment_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 : Use `await asyncio.gather(*tasks, return_exceptions=True)` for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_check_run_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 : Never use defensive checks on known library versions controlled in `pyproject.toml` - e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/runner_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 : Defensive checks ARE acceptable for lazy initialization where attributes explicitly start as `None` - check is legitimate for uninitialized attributes
Applied to files:
webhook_server/tests/test_check_run_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/tests/test_check_run_handler.pywebhook_server/libs/handlers/runner_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/utils/command_security.pywebhook_server/tests/test_command_security.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/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_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 : 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
Applied to files:
webhook_server/libs/handlers/runner_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: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
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.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 : 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/libs/github_api.py
🧬 Code graph analysis (7)
webhook_server/tests/test_custom_check_runs.py (2)
webhook_server/libs/handlers/check_run_handler.py (6)
set_custom_check_queued(247-249)set_custom_check_in_progress(251-253)set_custom_check_success(255-262)set_custom_check_failure(264-271)all_required_status_checks(488-523)get_check_run_text(328-381)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(626-705)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (1)
set_custom_check_queued(247-249)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(626-705)
webhook_server/tests/test_github_api.py (4)
webhook_server/tests/test_helpers.py (3)
get_value_side_effect(176-183)get_value_side_effect(199-206)get_value_side_effect(222-229)webhook_server/tests/test_webhook.py (1)
mock_config(319-329)webhook_server/tests/test_github_repository_and_webhook_settings.py (1)
mock_config(66-70)webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/tests/test_command_security.py (1)
webhook_server/utils/command_security.py (2)
CommandSecurityResult(13-17)validate_command_security(72-113)
webhook_server/libs/handlers/check_run_handler.py (3)
webhook_server/web/static/js/log_viewer.js (1)
status(12-12)webhook_server/tests/conftest.py (1)
github_webhook(111-131)webhook_server/tests/test_github_api.py (1)
logger(75-76)
webhook_server/libs/handlers/runner_handler.py (3)
webhook_server/utils/command_security.py (1)
validate_command_security(72-113)webhook_server/libs/handlers/check_run_handler.py (3)
set_custom_check_failure(264-271)set_custom_check_in_progress(251-253)get_check_run_text(328-381)webhook_server/utils/helpers.py (2)
format_task_fields(135-154)run_command(288-439)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
🪛 Ruff (0.14.10)
webhook_server/tests/test_custom_check_runs.py
271-271: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
306-306: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
338-338: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
387-387: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
416-416: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
432-432: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
467-467: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
534-534: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
572-572: Unused method argument: mock_pull_request
(ARG002)
595-595: Unused method argument: mock_pull_request
(ARG002)
620-620: Unused method argument: mock_pull_request
(ARG002)
654-654: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
728-728: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
764-764: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
812-812: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
846-846: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
webhook_server/libs/handlers/pull_request_handler.py
750-750: Logging statement uses f-string
(G004)
webhook_server/tests/test_github_api.py
1405-1405: Dynamically typed expressions (typing.Any) are disallowed in *_args
(ANN401)
1405-1405: Dynamically typed expressions (typing.Any) are disallowed in **_kwargs
(ANN401)
1405-1405: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
webhook_server/libs/handlers/check_run_handler.py
516-516: Logging statement uses f-string
(G004)
webhook_server/libs/handlers/runner_handler.py
634-634: Logging statement uses f-string
(G004)
643-643: Logging statement uses f-string
(G004)
838-838: Logging statement uses f-string
(G004)
854-854: Logging statement uses f-string
(G004)
868-868: Logging statement uses f-string
(G004)
875-875: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
827-827: Logging statement uses f-string
(G004)
🔇 Additional comments (29)
webhook_server/libs/handlers/issue_comment_handler.py (2)
5-5: Use ofCoroutinetype alias is correct and consistent (Severity: LOW)The new
from collections.abc import Coroutineimport matches thetasks: list[Coroutine[..., Any] | Task[Any]]annotation below and keeps typing explicit without affecting runtime behavior. No issues here.
449-454: Delegating retests toRunnerHandler.run_retestsis a good DRY/consolidation (Severity: LOW)Switching from per-test handling to:
if _supported_retests: await self.runner_handler.run_retests( supported_retests=_supported_retests, pull_request=pull_request, )centralizes all retest execution (including future custom checks) in
RunnerHandler, reducing duplicated logic and keepingIssueCommentHandler.process_retest_commandfocused on parsing/validation. Behavior for unsupported tests andautomergelabeling remains unchanged in this method.webhook_server/tests/test_check_run_handler.py (1)
49-50: Fixture now mirrorsGithubWebhook.custom_check_runssurface (Severity: LOW)Initializing
mock_webhook.custom_check_runs = []keeps the test fixture aligned with the production webhook object and prevents attribute errors when custom check logic inspects this list. This is the right place to wire in the new feature without altering test behavior.webhook_server/tests/test_issue_comment_handler.py (1)
43-44: Alignsmock_github_webhookwith newcustom_check_runsAPI (Severity: LOW)Adding
mock_webhook.custom_check_runs = []ensuresIssueCommentHandler(and downstream runners) can safely access the new configuration-driven custom check runs field during tests, avoiding brittlehasattrchecks in production.webhook_server/tests/test_pull_request_handler.py (1)
82-83: Pull request tests updated forcustom_check_runspresence (Severity: LOW)Defining
mock_webhook.custom_check_runs = []in the PR handler fixture correctly mirrors the real webhook object after introducing custom check runs, so handler logic can treat this attribute as always present without extra guards.webhook_server/utils/constants.py (1)
10-10:COMPLETED_STRstatus constant matches existing pattern (Severity: LOW)Introducing
COMPLETED_STR: str = "completed"is consistent with the other status constants and will reduce magic strings in check-run/custom-check flows. No functional risk here, and it improves readability and reuse.webhook_server/tests/test_github_api.py (1)
88-106: Custom-check-runs test config defaults are consistent (severity: LOW).Returning
[]for"custom-check-runs"in theseget_value_side_effecthelpers keepsGithubWebhook.custom_check_runsreliably iterable in tests and mirrors the production default behavior, so the new logic should integrate cleanly with the custom-check-runs feature. No changes needed here.Also applies to: 638-669
webhook_server/config/schema.yaml (1)
321-369: Custom-check-runs schema matches the runtime model (severity: LOW).The
custom-check-runsdefinition (requiredname/command,uv tool run --frompattern, boundedtimeout,requiredflag, constrainedtriggers, and constrainedsecrets) aligns with howGithubWebhook.custom_check_runs,PullRequestHandler, andRunnerHandler.run_custom_checkconsume this config. Thesecretspattern and required list also back up the secret-redaction behavior. Looks good.webhook_server/libs/github_api.py (1)
646-688: custom_check_runs loading and retest list integration look correct (severity: LOW).The
_repo_data_from_configchange ensuresself.custom_check_runsis always a list (defaulting to[]), and_current_pull_request_supported_retestsafely adds only entries with a definedname, logging a warning otherwise. This matches the new schema and howRunnerHandler.run_retestsandPullRequestHandlerconsume these names. No issues spotted.Also applies to: 805-831
webhook_server/libs/handlers/runner_handler.py (1)
626-706: Custom check execution path is secure and well-scoped (severity: LOW).
run_custom_check:
- Validates presence of
nameand fails fast with a clear log if missing.- Enforces command safety up front via
validate_command_security, turning failures into a dedicated failed check with explanatory output.- Collects secrets from env vars and passes only non-empty values into
run_command.redact_secrets, matching the schema’s “redact from logs” contract.- Runs inside
_checkout_worktreeand consistently updates check-run state via the newset_custom_check_*helpers.This aligns with the security and logging goals for custom checks.
webhook_server/libs/handlers/check_run_handler.py (1)
16-26: Custom check status helpers and required-check wiring look solid (severity: LOW).
- The new
set_custom_check_*helpers correctly wrapset_check_run_statususingCOMPLETED_STRplusSUCCESS_STR/FAILURE_STR, matching GitHub’s check-run lifecycle.all_required_status_checksnow folds incustom_check_runsflagged asrequired, and logs a warning whennameis missing instead of raising, which is consistent with other components and gives clean diagnostics for bad config.This integrates custom checks cleanly into the existing required-status and merge-eligibility logic.
Also applies to: 247-272, 511-519
webhook_server/utils/command_security.py (3)
1-17: LGTM! Clean module structure and NamedTuple definition.The docstring clearly states the purpose, imports are minimal and at the top, and
CommandSecurityResultis a well-typed NamedTuple providing clear semantics for validation results.
68-69: LGTM! Reasonable maximum command length.4096 characters is a sensible limit that prevents buffer-based attacks while allowing legitimate long commands with multiple arguments.
72-113: LOW: Empty and whitespace-only commands pass validation—verify this is intentional.The function returns
is_safe=Truefor empty strings and whitespace-only commands. While not a security vulnerability (executing an empty command is harmless), downstream code should handle this edge case. The current behavior is documented by tests, so this is just a note for awareness.The validation flow is well-structured: length → dangerous patterns → sensitive paths → null bytes → non-printable chars. The fail-fast approach with early returns is clean and efficient.
webhook_server/tests/test_custom_check_runs.py (6)
1-35: LGTM! Comprehensive test module with clear documentation.The module docstring clearly explains what's being tested (schema, handlers, runner, integration, retest). Imports are organized correctly at the top, and constants are imported from the appropriate module.
37-120: LOW: Schema validation tests document expectations rather than validate schema.These tests verify the structure of config dictionaries matches expectations, but don't actually run the YAML schema validator. This is acceptable as documentation tests, but consider adding a note that actual schema validation happens elsewhere.
The tests effectively document:
- Required fields (
name,command)- Optional fields with defaults (
timeout=600,required=true)- Valid trigger values
- Secrets configuration format
122-258: LGTM! Handler method tests are thorough and well-structured.Tests correctly:
- Mock
set_check_run_statusto verify delegation- Test all four custom check methods (queued, in_progress, success, failure)
- Verify correct status constants are passed
- Test
all_required_status_checksincludes/excludes custom checks based onrequiredflag- Properly reset cache before recalculation tests
260-518: LGTM! Runner handler tests cover critical execution paths.Excellent coverage of:
- Success and failure scenarios
- Checkout failure handling
- Default timeout application
- Worktree directory usage
- Secrets redaction (line 447 correctly uses
# pragma: allowlist secret)- Security validation rejection
The async context manager mocking pattern is correct and reusable.
826-859: LGTM! Edge case test for special characters correctly validates security blocking.The test verifies that commands containing
$variablesare blocked by security validation, with the check failing before command execution. This confirms the defense-in-depth approach works as intended.
793-824: HIGH: Test contradicts production behavior—run_commandcatchesTimeoutErrorand returns(False, "", message).Lines 823-824 mock
run_commandto raiseasyncio.TimeoutError, but the actualrun_commandimplementation (webhook_server/utils/helpers.py, lines 355-364) catchesTimeoutErrorand returns(False, "", f"Command timed out after {timeout}s")instead of propagating it.When
run_custom_checkreceivessuccess=Falsefromrun_command, it properly sets failure status viaset_custom_check_failure()(line 704). The test mock does not reflect this behavior—it should mockrun_commandto return(False, "", "Command timed out after 30s")instead of raising an exception.Likely an incorrect or invalid review comment.
webhook_server/tests/test_command_security.py (9)
1-16: LGTM! Clean test module setup.Imports are organized, the docstring clearly states purpose, and imports from
command_securitymodule are correct.
18-69: LGTM! Good coverage of valid command patterns.Both
uv tool runvariants and simple commands are tested. The parametrized approach ensures maintainability.
71-141: LGTM! Thorough shell injection prevention tests.Excellent coverage of:
- Semicolon, pipe, logical operators
- Command substitution (
$()and backticks)- Variable expansion (
${}and$VAR)- Redirections
- Background execution
- Newline escapes
- Dangerous builtins (eval, exec, source)
The comments explaining pattern matching order (e.g., line 85-86) are helpful for understanding test expectations.
144-195: LGTM! Dangerous command blocking tests are comprehensive.Good coverage of shell spawning, network tools, privilege escalation, file permission changes, and destructive operations. Case-insensitivity tests ensure bypass attempts are caught.
Line 156's comment correctly documents that
wget malicious.shmatches\bsh\bin the.shextension—this is intentional behavior since the wget pattern also triggers.
197-261: LGTM! Sensitive path access tests cover key vectors.System directories, process/system info, logs, boot partition, SSH keys, path traversal, environment files, config files, credentials, and private keys are all tested. Case-insensitivity tests included.
338-366: LOW: Consider adding a comment about empty command policy.Lines 341-351 test that empty and whitespace-only commands pass validation. While this matches the implementation, adding a brief comment explaining WHY this is acceptable behavior would help future maintainers:
- Empty commands are harmless to execute
- The caller (runner_handler) validates command presence separately
385-400: LGTM! Public API shape verification.Testing both named attribute access and index-based access on
CommandSecurityResultensures the NamedTuple contract is maintained.
402-454: LGTM! Real-world examples provide practical validation.The mix of valid and invalid commands with inline logic (lines 431-434) to determine expected outcome is a clean approach. Disguised malicious commands test defense-in-depth against obfuscation attempts.
456-484: LGTM! Performance and consistency tests add confidence.The 300 commands in <1 second threshold is reasonable for regex-based validation. Consistency test verifies deterministic behavior.
| # Shell metacharacters and operators that could be used for injection | ||
| DANGEROUS_SHELL_PATTERNS: list[tuple[str, str]] = [ | ||
| (r"[;&|]", "Shell operators (;, &, |) are not allowed"), | ||
| (r"\$\(", "Command substitution $() is not allowed"), | ||
| (r"`", "Backtick command substitution is not allowed"), | ||
| (r"\$\{", "Variable expansion ${} is not allowed"), | ||
| (r"\$[A-Za-z_]", "Variable expansion $VAR is not allowed"), | ||
| (r"[><]", "Redirections (>, <) are not allowed"), | ||
| (r"\|\|", "Logical OR (||) is not allowed"), | ||
| (r"&&", "Logical AND (&&) is not allowed"), | ||
| (r"\\n|\\r", "Newline escapes are not allowed"), | ||
| (r"\beval\b", "eval command is not allowed"), | ||
| (r"\bexec\b", "exec command is not allowed"), | ||
| (r"\bsource\b", "source command is not allowed"), | ||
| (r"\bsh\b", "sh command is not allowed"), | ||
| (r"\bbash\b", "bash command is not allowed"), | ||
| (r"\bzsh\b", "zsh command is not allowed"), | ||
| (r"\bcurl\b", "curl command is not allowed"), | ||
| (r"\bwget\b", "wget command is not allowed"), | ||
| (r"\bnc\b|\bnetcat\b", "netcat/nc command is not allowed"), | ||
| (r"\brm\s+-rf", "rm -rf is not allowed"), | ||
| (r"\bsudo\b", "sudo is not allowed"), | ||
| (r"\bsu\b", "su command is not allowed"), | ||
| (r"\bchmod\b", "chmod is not allowed"), | ||
| (r"\bchown\b", "chown is not allowed"), | ||
| (r"\bmkdir\s+-p\s+/", "Creating directories in root is not allowed"), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Consider pre-compiling regex patterns for performance.
These patterns are currently compiled on every call to validate_command_security. For a defense-in-depth module that may be called frequently, pre-compilation would reduce overhead.
Additionally, the pattern r"[;&|]" on line 22 will match a single & character, which could create false positives for commands with URLs containing query parameters (e.g., --url "https://example.com?a=1&b=2"). However, given the security-first approach and that uv tool run commands shouldn't typically need such URLs, this is acceptable.
🔎 Optional: Pre-compile patterns for better performance
import re
from typing import NamedTuple
# Pre-compile patterns for performance
_DANGEROUS_SHELL_PATTERNS: list[tuple[re.Pattern[str], str]] = [
(re.compile(r"[;&|]", re.IGNORECASE), "Shell operators (;, &, |) are not allowed"),
# ... remaining patterns
]🤖 Prompt for AI Agents
In webhook_server/utils/command_security.py around lines 20 to 46, the list
currently stores raw regex strings which are compiled repeatedly at runtime;
replace it with a module-level pre-compiled list (use re.compile for each
pattern, e.g. re.compile(r"...", re.IGNORECASE) as appropriate) and update the
type annotation to list[tuple[re.Pattern[str], str]]; also add the required
import for re at the top. Ensure any code that iterates this list
(validate_command_security) uses the compiled patterns directly (calling
pattern.search/match) rather than re.compile again.
| # Sensitive paths that should never be accessed | ||
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | ||
| (r"/etc/", "Access to /etc/ is not allowed"), | ||
| (r"/root/", "Access to /root/ is not allowed"), | ||
| (r"~/.ssh", "Access to SSH keys is not allowed"), | ||
| (r"/proc/", "Access to /proc/ is not allowed"), | ||
| (r"/sys/", "Access to /sys/ is not allowed"), | ||
| (r"/dev/", "Access to /dev/ is not allowed"), | ||
| (r"/var/log/", "Access to /var/log/ is not allowed"), | ||
| (r"/boot/", "Access to /boot/ is not allowed"), | ||
| (r"\.\.\/", "Path traversal (..) is not allowed"), | ||
| (r"\.env", "Access to .env files is not allowed"), | ||
| (r"config\.yaml", "Access to config.yaml is not allowed"), | ||
| (r"credentials", "Access to credentials files is not allowed"), | ||
| (r"\.pem\b", "Access to PEM files is not allowed"), | ||
| (r"\.key\b", "Access to key files is not allowed"), | ||
| (r"id_rsa", "Access to SSH private keys is not allowed"), | ||
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Path traversal pattern only catches forward slashes.
Line 58's pattern r"\.\.\/ catches ../ but not ..\\ (Windows-style) or standalone .. at command end. While this server likely runs on Linux, defense-in-depth should consider both.
Also, line 52's ~/.ssh works correctly but the tilde isn't escaped—it's benign here since ~ only has special meaning inside character classes [~], but escaping it as \~ would be more explicit.
🔎 Proposed enhancement for path traversal detection
- (r"\.\.\/", "Path traversal (..) is not allowed"),
+ (r"\.\.[/\\]", "Path traversal (..) is not allowed"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Sensitive paths that should never be accessed | |
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | |
| (r"/etc/", "Access to /etc/ is not allowed"), | |
| (r"/root/", "Access to /root/ is not allowed"), | |
| (r"~/.ssh", "Access to SSH keys is not allowed"), | |
| (r"/proc/", "Access to /proc/ is not allowed"), | |
| (r"/sys/", "Access to /sys/ is not allowed"), | |
| (r"/dev/", "Access to /dev/ is not allowed"), | |
| (r"/var/log/", "Access to /var/log/ is not allowed"), | |
| (r"/boot/", "Access to /boot/ is not allowed"), | |
| (r"\.\.\/", "Path traversal (..) is not allowed"), | |
| (r"\.env", "Access to .env files is not allowed"), | |
| (r"config\.yaml", "Access to config.yaml is not allowed"), | |
| (r"credentials", "Access to credentials files is not allowed"), | |
| (r"\.pem\b", "Access to PEM files is not allowed"), | |
| (r"\.key\b", "Access to key files is not allowed"), | |
| (r"id_rsa", "Access to SSH private keys is not allowed"), | |
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | |
| ] | |
| # Sensitive paths that should never be accessed | |
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | |
| (r"/etc/", "Access to /etc/ is not allowed"), | |
| (r"/root/", "Access to /root/ is not allowed"), | |
| (r"~/.ssh", "Access to SSH keys is not allowed"), | |
| (r"/proc/", "Access to /proc/ is not allowed"), | |
| (r"/sys/", "Access to /sys/ is not allowed"), | |
| (r"/dev/", "Access to /dev/ is not allowed"), | |
| (r"/var/log/", "Access to /var/log/ is not allowed"), | |
| (r"/boot/", "Access to /boot/ is not allowed"), | |
| (r"\.\.[/\\]", "Path traversal (..) is not allowed"), | |
| (r"\.env", "Access to .env files is not allowed"), | |
| (r"config\.yaml", "Access to config.yaml is not allowed"), | |
| (r"credentials", "Access to credentials files is not allowed"), | |
| (r"\.pem\b", "Access to PEM files is not allowed"), | |
| (r"\.key\b", "Access to key files is not allowed"), | |
| (r"id_rsa", "Access to SSH private keys is not allowed"), | |
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | |
| ] |
🤖 Prompt for AI Agents
In webhook_server/utils/command_security.py around lines 48 to 66, the
path-traversal regex r"\.\.\/" only matches "../" (forward slash) and the
"~/.ssh" pattern leaves tilde unescaped; update the traversal detection to also
match Windows-style backslashes and standalone/terminal ".." instances (e.g.,
patterns that match "..\", ".." at end or by itself) and escape the tilde in the
SSH pattern for clarity; keep patterns anchored or use alternation to cover
"../", "..\", and trailing ".." cases while preserving existing error messages.
Remove complex security validation layer and adopt a trust-based approach: - Delete command_security.py and related security validation logic - Simplify schema to only name, command, env fields - Add shutil.which() check to gracefully skip unavailable commands - Add set_custom_check_skipped() for handling missing commands - Update tests to reflect simplified validation model This change assumes users will configure safe commands and gracefully handles cases where commands are not found instead of blocking them.
Simplify env configuration by specifying only variable names instead of key-value pairs. Values are read from server environment at runtime. Changes: - Schema: env changed from object to array of strings - Handler: reads env var values from os.environ - Tests: updated for new env list format
Added example showing environment variable reference in custom-check-runs commands to improve documentation clarity.
Remove special handling for custom checks - they now fail naturally like built-in checks (tox, pre-commit, etc.) when commands are not found. Changes: - runner_handler.py: Removed skip logic for missing commands - check_run_handler.py: Removed set_custom_check_skipped method - schema.yaml: Simplified description to reflect new behavior - test_custom_check_runs.py: Removed skip/trigger/timeout tests
Allows env entries in custom check run configuration to use two formats:
- Variable name only (VAR) - reads from server environment
- Variable with explicit value (VAR=value) - uses provided value
This enables mixed usage like:
env:
- PYTHONPATH
- DEBUG=true
Custom checks now behave exactly like built-in checks (tox, pre-commit): - Run on all PR events (no trigger filtering) - Always required for merge (no required field)
- Validate name, command, and executable existence at config load time - Remove duplicate validation from 5 locations - Remove configurable timeout (use default like built-in checks) - Check if command executable exists on server before accepting check
- Use functools.partial instead of nested function in run_retests() - Add custom_check_runs to mock fixtures in test files
Clean up remnants from old logging patterns not removed during structured logging migration. Replaces logger.step (undefined) and format_task_fields (undefined) with standard logger.info calls. Affected handlers: - pull_request_handler.py: setup tasks and CI/CD tasks logging - runner_handler.py: custom check execution logging
…add-plugin-option
- Remove "variable name only" env var option, require explicit VAR=value format - Consolidate command validation in _validate_custom_check_runs - Remove redundant COMPLETED_STR from custom check success/failure methods - Add tests for validation edge cases and duration formatting - Achieve 90% test coverage
When passing env to asyncio.create_subprocess_exec(), it REPLACES the entire environment instead of extending it. Without os.environ.copy(), the subprocess wouldn't have PATH, HOME, or other essential variables, causing commands like 'uv', 'python', etc. to fail. - Change env_dict from empty dict to os.environ.copy() - Add explanatory comment for future maintainers - Update tests to verify environment inheritance
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @webhook_server/config/schema.yaml:
- Around line 321-370: The test fixture valid_full_config in
webhook_server/tests/test_config_schema.py is missing the optional
custom-check-runs entries; add a couple of examples under the repository
configuration (one check without env and one with env entries following the
schema pattern "^[^=]+=.+$") so the full schema validation exercises
custom-check-runs; ensure examples match the schema fields (name, command,
optional env array) and thus exercise
GithubWebhook._validate_custom_check_runs() and the RunnerHandler runtime
expectations during the full config test.
In @webhook_server/libs/github_api.py:
- Around line 625-630: The config allows custom check names that can collide
with built-in retest keys; update _validate_custom_check_runs to detect if a
custom check's name equals any built-in retest constant (e.g., TOX_STR,
PRE_COMMIT_STR, BUILD_CONTAINER_STR, PYTHON_MODULE_INSTALL_STR,
CONVENTIONAL_TITLE_STR) and either skip/raise/log a warning and omit that entry
(so it doesn't override built-ins used by RunnerHandler.run_retests); ensure
_repo_data_from_config keeps using the validated list and that the validation
still trims/validates command and uses shutil.which as before.
In @webhook_server/libs/handlers/runner_handler.py:
- Around line 486-557: run_custom_check currently injects env values from
check_config["env"] into env_dict but never tells the redaction pipeline about
those secrets, so values like TOKEN=... can leak in logs/output; update
run_custom_check to derive a list of secret strings from the env entries
(extract the right-hand side values from check_config["env"] entries that
contain '=') and pass that list into run_command (e.g., via its redact/secret
parameter) and into get_check_run_text (or into whatever output-redaction call
is used) so both subprocess logging and check-run output are redacted; ensure
you still start from os.environ.copy() when building env_dict and only include
non-malformed env entries while logging the var names as before.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_github_api.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain backward compatibility for user-facing configuration files (
config.yaml,.github-webhook-server.yaml) - configuration schema changes must support old formats or provide migration
Files:
webhook_server/config/schema.yaml
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/config/schema.yaml: Mask sensitive data in logs by default (setmask-sensitive-data: truein configuration)
Configuration schema changes should be tested withuv run pytest webhook_server/tests/test_config_schema.py -v
Files:
webhook_server/config/schema.yaml
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/github_api.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.py
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Do NOT use defensive checks for architecture guarantees likerepository_datawhich is ALWAYS set before handlers instantiate
Handler classes must implement__init__(self, github_webhook, ...)andprocess_event(event_data)pattern
Handlers must useself.github_webhook.unified_apifor all GitHub operations, not direct PyGithub calls
Usectx.start_step(),ctx.complete_step(),ctx.fail_step()for structured webhook execution tracking
Files:
webhook_server/libs/handlers/runner_handler.py
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/config/schema.yamlwebhook_server/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to **/*.{yaml,yml} : Maintain backward compatibility for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Applied to files:
webhook_server/tests/test_github_api.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_github_api.pywebhook_server/tests/test_custom_check_runs.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_github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/github_api.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/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.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/runner_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/runner_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/runner_handler.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
Applied to files:
webhook_server/libs/github_api.py
🧬 Code graph analysis (3)
webhook_server/libs/handlers/runner_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (4)
set_custom_check_in_progress(220-222)get_check_run_text(264-317)set_custom_check_failure(228-230)set_custom_check_success(224-226)webhook_server/utils/helpers.py (1)
run_command(266-417)
webhook_server/tests/test_custom_check_runs.py (1)
webhook_server/libs/handlers/check_run_handler.py (5)
set_custom_check_queued(216-218)set_custom_check_in_progress(220-222)set_custom_check_success(224-226)set_custom_check_failure(228-230)all_required_status_checks(424-457)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
🪛 Ruff (0.14.10)
webhook_server/libs/handlers/runner_handler.py
500-500: Logging statement uses f-string
(G004)
533-533: Logging statement uses f-string
(G004)
536-537: Logging statement uses f-string
(G004)
552-552: Logging statement uses f-string
(G004)
555-555: Logging statement uses f-string
(G004)
656-656: Logging statement uses f-string
(G004)
679-679: Logging statement uses f-string
(G004)
681-681: Logging statement uses f-string
(G004)
688-688: Logging statement uses f-string
(G004)
webhook_server/tests/test_custom_check_runs.py
206-206: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
240-240: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
269-269: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
317-317: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
333-333: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
346-346: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
381-381: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
411-411: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
447-447: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
489-489: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
525-525: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
599-599: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
842-842: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
889-889: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
923-923: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
webhook_server/libs/github_api.py
813-813: Logging statement uses f-string
(G004)
822-822: Logging statement uses f-string
(G004)
828-828: Logging statement uses f-string
(G004)
832-832: Logging statement uses f-string
(G004)
834-834: Logging statement uses f-string
(G004)
🔇 Additional comments (6)
webhook_server/tests/test_github_api.py (2)
89-107: Custom-check-runs mocks are aligned with production expectations (Severity: LOW).Both
get_value_side_effecthelpers now return[]for"custom-check-runs", which matches the schema (array) and whatGithubWebhook._repo_data_from_configexpects when calling_validate_custom_check_runs. This keeps the tests realistic and prevents accidentalNone/type drift when exercising clone- and config-related paths. No changes needed.Also applies to: 649-681
1405-1450: Empty checkout_ref test correctly pins _clone_repository contract (Severity: LOW).
test_clone_repository_empty_checkout_refcleanly verifies that_clone_repositoryrejects an emptycheckout_refwith aValueError, using a minimalConfigmock and a tailoredget_value_side_effectthat still supports"custom-check-runs". This is a good guard against regressions in argument validation.webhook_server/tests/test_custom_check_runs.py (3)
34-193: Good coverage of CheckRunHandler custom-check plumbing (Severity: LOW).The
TestCustomCheckRunsSchemaValidationandTestCheckRunHandlerCustomCheckMethodsclasses give nice, focused checks that:
- Sanity-check example configs (including multiline and
envstructure).- Verify that each
set_custom_check_*helper delegates correctly toset_check_run_status.- Confirm that
all_required_status_checksincludes all configured custom check names.This tightly couples tests to the public behavior rather than internals and will catch regressions in how custom checks surface into required-status logic.
195-473: RunnerHandler custom-check tests thoroughly exercise env and worktree behavior (Severity: LOW).The
TestRunnerHandlerCustomCheckblock:
- Verifies success/failure paths, checkout failures, and that commands run in the worktree
cwd.- Exercises env handling (explicit
VAR=value, missingenv→env=None, invalid entries skipped) and asserts inheritedPATH, which is critical givenasyncio.create_subprocess_execreplaces the environment whenenvis passed.- Uses AsyncMock’d
_checkout_worktreeandrun_command, so tests stay fast and hermetic.This is exactly the sort of behavior-level coverage that makes future refactors of
run_custom_checksafe.
626-831: _validate_custom_check_runs tests match implementation branches very well (Severity: LOW).
TestValidateCustomCheckRunshits all key branches of_validate_custom_check_runs:
- Missing/empty/whitespace-only
command, missingname, and non-existent executables.- Multiple-failure scenarios with summary-warning assertions.
- Happy paths including multiline commands and full-path executables.
Using a
Mockas theGithubWebhookinstance and patchingshutil.whichkeeps these tests focused purely on validation behavior and logging. Nice alignment with the production logic.webhook_server/libs/handlers/runner_handler.py (1)
648-688: run_retests mapping and defensive behavior look good (Severity: LOW).
- The
_retests_to_func_mapcentralizes all built-in runners and is extended with partials for custom checks based ongithub_webhook.custom_check_runs, which keeps the retest orchestration in one place.- Using
.get()and logging an error on unknown_testnames (instead of raising) makes the retest flow more robust to future config changes, while still surfacing issues clearly.- Running all mapped runners via
asyncio.create_task+gather(..., return_exceptions=True)is appropriate here; exceptions are logged and don’t silently disappear.This matches the intended behavior from the earlier review discussion about avoiding
KeyErrorwhile still failing loudly enough in logs.
- Add custom-check-runs examples to valid_full_config test fixture - Detect and reject custom check names that collide with built-ins - Mask env-sourced secrets in logs/output for security - Add test for built-in name collision detection
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @webhook_server/libs/github_api.py:
- Around line 787-851: The _validate_custom_check_runs function currently allows
duplicate custom check names which later cause silent overrides; fix this by
tracking seen names (e.g., a local set seen_names) while iterating raw_checks
and if check.get("name") is already in seen_names log a warning and skip the
duplicate (keep the first seen entry), otherwise add the name to seen_names and
validated_checks; also ensure final info/warning summary messages still reflect
the number skipped due to duplicates and collisions so callers like
RunnerHandler.run_retests receive a deterministic, unique-name list.
In @webhook_server/tests/test_github_api.py:
- Around line 1654-1712: The test
test_validate_custom_check_runs_builtin_collision currently uses wide Any typing
and an unused logger param; change the nested helper signature to use
object-based varargs and a precise return type (e.g. def
get_value_side_effect(value: str, *_args: object, **_kwargs: object) ->
Optional[object]: and return the same list/dict/None values), and rename the
test parameter logger to _logger (or use the fixture instead) so the unused Mock
no longer triggers ARG002; keep the helper behavior identical and leave the rest
of the test untouched (references:
test_validate_custom_check_runs_builtin_collision, get_value_side_effect,
logger/_logger).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_github_api.py
🧰 Additional context used
📓 Path-based instructions (5)
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.py
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/handlers/runner_handler.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_github_api.pywebhook_server/tests/test_config_schema.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Do NOT use defensive checks for architecture guarantees likerepository_datawhich is ALWAYS set before handlers instantiate
Handler classes must implement__init__(self, github_webhook, ...)andprocess_event(event_data)pattern
Handlers must useself.github_webhook.unified_apifor all GitHub operations, not direct PyGithub calls
Usectx.start_step(),ctx.complete_step(),ctx.fail_step()for structured webhook execution tracking
Files:
webhook_server/libs/handlers/runner_handler.py
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_config_schema.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/handlers/runner_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/github_api.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/libs/github_api.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/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Do NOT use hasattr() checks for library versions we control in `pyproject.toml` (PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
Applied to files:
webhook_server/libs/github_api.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/github_api.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g., `user.node_id`, `user.type`, `sender`) - let KeyError surface legitimate bugs
Applied to files:
webhook_server/libs/github_api.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/github_api.pywebhook_server/tests/test_github_api.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/libs/github_api.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_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/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes `| None` or `Optional`)
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Access configuration using `Config(repository='org/repo-name')` and `config.get_value('setting-name', default_value)`
Applied to files:
webhook_server/tests/test_config_schema.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/libs/handlers/runner_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/runner_handler.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
🧬 Code graph analysis (3)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/tests/test_github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/libs/handlers/runner_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (2)
set_custom_check_in_progress(220-222)get_check_run_text(264-317)webhook_server/utils/helpers.py (1)
run_command(266-417)
🪛 Ruff (0.14.10)
webhook_server/libs/github_api.py
822-822: Logging statement uses f-string
(G004)
828-828: Logging statement uses f-string
(G004)
837-837: Logging statement uses f-string
(G004)
843-843: Logging statement uses f-string
(G004)
847-847: Logging statement uses f-string
(G004)
849-849: Logging statement uses f-string
(G004)
webhook_server/tests/test_github_api.py
1655-1655: Unused method argument: logger
(ARG002)
1663-1663: Dynamically typed expressions (typing.Any) are disallowed in *_args
(ANN401)
1663-1663: Dynamically typed expressions (typing.Any) are disallowed in **_kwargs
(ANN401)
1663-1663: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
webhook_server/libs/handlers/runner_handler.py
500-500: Logging statement uses f-string
(G004)
537-537: Logging statement uses f-string
(G004)
540-541: Logging statement uses f-string
(G004)
557-557: Logging statement uses f-string
(G004)
560-560: Logging statement uses f-string
(G004)
661-661: Logging statement uses f-string
(G004)
684-684: Logging statement uses f-string
(G004)
686-686: Logging statement uses f-string
(G004)
693-693: Logging statement uses f-string
(G004)
🔇 Additional comments (4)
webhook_server/libs/github_api.py (1)
625-629: Custom check config loading and retest wiring look solid (Severity: LOW)The new
custom-check-runsloading in_repo_data_from_configand its use in_current_pull_request_supported_retestare consistent:
config.get_value(..., return_on_none=[])guaranteesself.custom_check_runsis always a list._current_pull_request_supported_retestonly consumes validated entries, so/retestsurfaces only checks that passed_validate_custom_check_runs.No changes needed here; behavior is predictable and matches the schema/runner expectations.
Also applies to: 745-771
webhook_server/tests/test_config_schema.py (1)
81-88: Valid config fixture now exercises custom-check-runs structure (Severity: LOW)Including
custom-check-runswith realisticuv tool runcommands and anenvlist keeps the “full config” fixture in sync with the new schema and runtime expectations. No changes needed.webhook_server/tests/test_github_api.py (1)
88-107: Config mocks updated for custom-check-runs are appropriate (Severity: LOW)The added
if value == "custom-check-runs": return []branches in the variousget_value_side_effecthelpers ensure that newly introduced config access doesn’t break existing tests and thatGithubWebhook._repo_data_from_configalways sees a list in scenarios where custom checks are irrelevant.This keeps tests focused on their primary behavior without being coupled to the new feature. No changes required here.
Also applies to: 649-680, 1405-1428
webhook_server/libs/handlers/runner_handler.py (1)
3-8: Custom check execution and retest orchestration are well-structured (Severity: LOW)
run_custom_check:
- Correctly assumes
name/commandwere validated at load time and uses them directly.- Runs in a per-PR worktree, mirroring existing tox/pre-commit flows.
- Builds
env_dictfromos.environ.copy()so PATH/HOME/etc. are preserved (critical becauseenvreplaces the entire environment for subprocesses).- Extracts secret values from
enventries and feeds them intorun_command.redact_secrets, so log output won’t leak user-provided secrets.
run_retests:
- Centralizes the mapping from check names to runner coroutines, then extends it with partials for each custom check using
self.github_webhook.custom_check_runs.- Handles unknown test names gracefully with an error log instead of raising
KeyError, while still failing fast for genuine programming errors inside tasks viaasyncio.gather(..., return_exceptions=True)and an error log.The design cleanly integrates custom checks into the existing runner surface without changing the semantics of the built-in checks. I don’t see any blocking issues here.
Also applies to: 486-562, 653-693
- Add seen_names set to detect duplicate custom check names - Skip duplicates with warning log (first occurrence wins) - Change test helper typing from Any to object for unused varargs - Remove unused logger fixture parameter from test functions - Add test for duplicate name detection behavior Addresses CodeRabbit review comments.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @webhook_server/tests/test_github_api.py:
- Around line 1654-1711: The local test helper get_value_side_effect currently
uses a broad -> Any return type; change its signature to a precise union like ->
Union[List[Dict[str, Any]], Dict[str, Any], None] (and import required typing
names) so it matches the earlier fix pattern used at line ~1423; update the
annotation on get_value_side_effect in
test_validate_custom_check_runs_builtin_collision and run the tests to ensure
ANN401 is resolved while keeping the same returned values (list of dicts for
"custom-check-runs", {} for "container"/"pypi", and None otherwise).
- Around line 1712-1753: The local test helper get_value_side_effect in
test_validate_custom_check_runs_duplicate_names currently types its return as
Any; tighten the return annotation to a precise union reflecting the actual
branches (when value == "custom-check-runs" return List[Dict[str, str]], when
value == "container" or "pypi" return Dict, otherwise None). Update the
signature of get_value_side_effect (and import required typing names) so its
return type is Optional[Union[List[Dict[str, str]], Dict[str, Any]]], keeping
the same behavior used by GithubWebhook and
mock_config.return_value.get_value.side_effect.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.py
🧰 Additional context used
📓 Path-based instructions (4)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_github_api.pywebhook_server/libs/github_api.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_github_api.py
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/github_api.py
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_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/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes `| None` or `Optional`)
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Applied to files:
webhook_server/tests/test_github_api.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_github_api.pywebhook_server/libs/github_api.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_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Do NOT use hasattr() checks for library versions we control in `pyproject.toml` (PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
Applied to files:
webhook_server/libs/github_api.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/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/libs/github_api.py
🧬 Code graph analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
🪛 Ruff (0.14.10)
webhook_server/tests/test_github_api.py
1661-1661: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
1719-1719: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
webhook_server/libs/github_api.py
823-823: Logging statement uses f-string
(G004)
828-828: Logging statement uses f-string
(G004)
835-835: Logging statement uses f-string
(G004)
844-844: Logging statement uses f-string
(G004)
850-850: Logging statement uses f-string
(G004)
854-854: Logging statement uses f-string
(G004)
856-856: Logging statement uses f-string
(G004)
🔇 Additional comments (6)
webhook_server/tests/test_github_api.py (3)
103-104: LGTM!Fixture correctly returns an empty list for
custom-check-runsconfig key, matching the expected list-based representation.
678-679: LGTM!Consistent with the fixture update - returns empty list for custom-check-runs.
1422-1428: LGTM!Typing now uses
objectfor varargs and a union return typelist[object] | None. This satisfies ANN401 and aligns with the earlier fix pattern.webhook_server/libs/github_api.py (3)
625-630: LGTM - config loading follows established patterns.Loading with
return_on_none=[]ensures a consistent list type, and validation is delegated to_validate_custom_check_runs(). This matches the project's config access patterns.
763-770: LGTM - custom checks correctly added to retest support.The comment accurately notes that
nameis guaranteed by validation. Appending to the supported retest list enables/retest <custom-check-name>functionality.
787-858: Solid validation implementation - well structured with proper fail-fast behavior.The method correctly implements:
- Name/command field validation - skips entries missing required fields
- Built-in collision detection - prevents overriding
tox,pre-commit, etc.- Duplicate name detection - first-occurrence-wins semantics
- Executable verification -
shutil.which()catches missing tools at config load timeWHY this matters: Validating at load time prevents cryptic runtime failures when a custom check is triggered but its command doesn't exist. Users get immediate, actionable feedback.
One minor observation:
BUILTIN_CHECK_NAMESis defined inline. If you need to reference this set in tests or other modules, consider promoting it to module-level. Not blocking.The static analysis G004 warnings about f-string logging are false positives. As per coding guidelines, f-strings are the established logging style in this codebase.
|
pushing another fix now; review it thoroughly and not in chunks |
|
@rnetser I'll review all the changes in this PR thoroughly. Let me analyze the entire changeset including the recent fix you pushed. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_config_schema.py (1)
81-92:custom-check-runsfixture shape matches the new schemaSeverity: LOW. The
custom-check-runsentries use the expected structure (name,command, optionalenvasKEY=valuestrings), sovalid_full_configremains a good positive (“happy‑path”) schema test. If you don’t already have them elsewhere, consider adding focused tests that exercise invalid shapes (e.g., missingname/command, malformedenventries) so schema validation around custom checks is explicitly covered, not just via this full‑config smoke test.
🤖 Fix all issues with AI Agents
In @webhook_server/libs/handlers/issue_comment_handler.py:
- Around line 439-444: The duplicate “test name → runner” dispatch in
IssueCommentHandler should be removed and centralized by delegating to
RunnerHandler.run_retests; update the code in IssueCommentHandler (where
_supported_retests is computed) to call await
self.runner_handler.run_retests(supported_retests=_supported_retests,
pull_request=pull_request) instead of reimplementing dispatch logic, ensuring
_supported_retests is still filtered prior to the call so unsupported tests
remain excluded and existing automerge/labeling semantics in
RunnerHandler.run_retests are preserved.
In @webhook_server/libs/handlers/runner_handler.py:
- Around line 653-693: The tasks list is over-annotated; change the type
annotation on the local variable tasks in run_retests to list[Task[Any]] (remove
Coroutine[..., Any, Any] union) because you always call asyncio.create_task
which returns Task; update the declaration of tasks in the run_retests method
(refer to the variable name tasks and the function run_retests) accordingly to
simplify the type and keep typing accurate.
In @webhook_server/tests/test_github_api.py:
- Around line 1654-1711: The nested helper get_value_side_effect currently
annotated as -> Any; tighten its return type to reflect actual branches (list of
dicts for "custom-check-runs", dict for "container"/"pypi", or None) by changing
the signature to something like -> Optional[Union[List[Dict[str, str]],
Dict[str, Any]]] and add the required typing imports (e.g., Optional, Union,
List, Dict, Any) at the top of the test file; keep the function body unchanged.
- Around line 1712-1753: The local helper get_value_side_effect in
test_validate_custom_check_runs_duplicate_names currently types its return as
Any; narrow it to match actual returns by changing its annotation to return
Optional[Union[List[Dict[str, str]], Dict[str, str]]], and add the corresponding
typing imports (Optional, Union, List, Dict) at the top of the test file; keep
the signature (value: str, *_args: object, **_kwargs: object) and leave the body
unchanged so the test still returns the list of dicts, {} or None and satisfies
ANN401.
- Around line 1423-1428: The test's get_value_side_effect currently only handles
"custom-check-runs" and returns None for everything else, which can break if
GithubWebhook.__init__ later requires other config keys; update the
mock_config.get_value.side_effect used in this test (the get_value_side_effect
function) to mirror the broader fixture's branches or otherwise return sensible
defaults for other known config keys rather than None, ensuring
_clone_repository and GithubWebhook.__init__ behave as expected during test
setup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_context.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_push_handler.pywebhook_server/tests/test_runner_handler.py
🧰 Additional context used
📓 Path-based instructions (7)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_push_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/github_api.pywebhook_server/tests/test_context.pywebhook_server/tests/test_custom_check_runs.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_context.pywebhook_server/tests/test_custom_check_runs.py
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Do NOT use defensive checks for architecture guarantees likerepository_datawhich is ALWAYS set before handlers instantiate
Handler classes must implement__init__(self, github_webhook, ...)andprocess_event(event_data)pattern
Handlers must useself.github_webhook.unified_apifor all GitHub operations, not direct PyGithub calls
Usectx.start_step(),ctx.complete_step(),ctx.fail_step()for structured webhook execution tracking
Files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain backward compatibility for user-facing configuration files (
config.yaml,.github-webhook-server.yaml) - configuration schema changes must support old formats or provide migration
Files:
webhook_server/config/schema.yaml
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/config/schema.yaml: Mask sensitive data in logs by default (setmask-sensitive-data: truein configuration)
Configuration schema changes should be tested withuv run pytest webhook_server/tests/test_config_schema.py -v
Files:
webhook_server/config/schema.yaml
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
🧠 Learnings (27)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_context.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/config/schema.yamlwebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotation `attr: SomeType | None = None`
Applied to files:
webhook_server/tests/test_runner_handler.pywebhook_server/tests/test_check_run_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/tests/test_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/runner_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_runner_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_push_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/libs/github_api.pywebhook_server/tests/test_context.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks in destructors (`__del__`) should use hasattr() to check for attributes that may not exist if initialization failed
Applied to files:
webhook_server/tests/test_check_run_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_push_handler.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes `| None` or `Optional`)
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Applied to files:
webhook_server/tests/test_github_api.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_github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.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/tests/test_github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.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.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_push_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to **/*.{yaml,yml} : Maintain backward compatibility for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration
Applied to files:
webhook_server/config/schema.yaml
📚 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/libs/handlers/issue_comment_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/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/app.py : Use `create_context()` to initialize webhook context in app.py with hook_id, event_type, repository, action, sender, api_user
Applied to files:
webhook_server/tests/test_context.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/utils/context.py : Webhook logs should be written to `{config.data_dir}/logs/webhooks_YYYY-MM-DD.json` in pretty-printed JSON format (2-space indentation) with daily rotation based on UTC date
Applied to files:
webhook_server/tests/test_context.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `ctx.start_step()`, `ctx.complete_step()`, `ctx.fail_step()` for structured webhook execution tracking
Applied to files:
webhook_server/tests/test_context.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Use `get_logger_with_params()` for logging with structured context (name, repository, hook_id)
Applied to files:
webhook_server/tests/test_context.py
🧬 Code graph analysis (5)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (1)
set_custom_check_queued(216-218)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(486-561)
webhook_server/libs/handlers/issue_comment_handler.py (1)
webhook_server/libs/handlers/runner_handler.py (1)
run_retests(653-693)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/tests/test_context.py (1)
webhook_server/utils/context.py (3)
_format_duration(48-78)WebhookContext(82-372)_build_summary(297-332)
webhook_server/tests/test_custom_check_runs.py (2)
webhook_server/libs/github_api.py (1)
_validate_custom_check_runs(787-858)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(486-561)
🪛 Ruff (0.14.10)
webhook_server/tests/test_github_api.py
1661-1661: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
1719-1719: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
webhook_server/libs/handlers/pull_request_handler.py
629-629: Logging statement uses f-string
(G004)
663-663: Logging statement uses f-string
(G004)
webhook_server/libs/handlers/runner_handler.py
500-500: Logging statement uses f-string
(G004)
537-537: Logging statement uses f-string
(G004)
540-541: Logging statement uses f-string
(G004)
557-557: Logging statement uses f-string
(G004)
560-560: Logging statement uses f-string
(G004)
661-661: Logging statement uses f-string
(G004)
684-684: Logging statement uses f-string
(G004)
686-686: Logging statement uses f-string
(G004)
693-693: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
823-823: Logging statement uses f-string
(G004)
828-828: Logging statement uses f-string
(G004)
835-835: Logging statement uses f-string
(G004)
844-844: Logging statement uses f-string
(G004)
850-850: Logging statement uses f-string
(G004)
854-854: Logging statement uses f-string
(G004)
856-856: Logging statement uses f-string
(G004)
webhook_server/tests/test_custom_check_runs.py
206-206: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
240-240: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
269-269: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
317-317: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
333-333: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
346-346: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
381-381: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
411-411: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
447-447: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
489-489: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
525-525: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
599-599: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
842-842: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
889-889: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
923-923: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
⏰ 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). (9)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: conventional-title
- GitHub Check: build-container
- GitHub Check: python-module-install
- GitHub Check: pre-commit
🔇 Additional comments (21)
webhook_server/tests/test_check_run_handler.py (1)
29-51: Initializecustom_check_runson the webhook mock (looks correct)Severity: LOW. This keeps the
GithubWebhooktest double structurally aligned with the real object and preventsAttributeErroronce production code starts readinggithub_webhook.custom_check_runs. No changes needed here.webhook_server/tests/test_pull_request_handler.py (1)
46-86:custom_check_runsfixture wiring for PR handler is consistent and safeSeverity: LOW. Adding
mock_webhook.custom_check_runs = []keeps theGithubWebhookspec‑mock in sync with the real API and with other handler tests that now rely on repository‑configured custom checks. This avoids brittle AttributeErrors without changing test behavior. No further changes needed.webhook_server/tests/test_push_handler.py (1)
29-48: PushHandler webhook mock includescustom_check_runs(future‑proofing is fine)Severity: LOW. Initializing
custom_check_runsto an empty list on the push webhook mock is harmless today and keeps the fixture compatible with the expandedGithubWebhookAPI so future logic touching custom checks won’t break tests. No action required.webhook_server/tests/test_issue_comment_handler.py (1)
27-46: IssueComment webhook mock now exposescustom_check_runs(aligned with other handlers)Severity: LOW. Defining
custom_check_runson this fixture makes it consistent with other handler tests and with the realGithubWebhooksurface, preventing surprises if comment‑driven flows start interacting with custom checks. No further changes needed.webhook_server/tests/test_runner_handler.py (1)
39-39: Initializecustom_check_runson the mock webhook (LOW – correctness/fixtures).Setting
mock_webhook.custom_check_runs = []keeps the fixture aligned with the realGithubWebhooksurface, and avoids attribute errors whenRunnerHandleriterates custom checks. This is the right default for “no custom checks” in these tests.webhook_server/libs/handlers/pull_request_handler.py (2)
622-629: Custom check queuing mirrors built‑in checks (LOW – behavior alignment).The Stage‑1 block that iterates
self.github_webhook.custom_check_runsand callsset_custom_check_queued(name=check_name)is consistent with how built‑in checks are queued. Relying on prior validation (GithubWebhook._validate_custom_check_runs) instead of re‑checking keys here is correct and keeps handler logic lean.
653-663: Wiring custom checks into CI/CD execution path (LOW – behavior alignment).Adding each
run_custom_check(pull_request=..., check_config=custom_check)toci_taskscleanly reuses the existing async fan‑out pattern and ensures custom checks participate in the same error‑logging and context‑step lifecycle as built‑in checks. The extra info log at Line 663 makes the two‑stage workflow (“setup” vs “CI/CD”) more observable without changing semantics.webhook_server/tests/test_github_api.py (2)
88-107: Extend shared config side effect forcustom-check-runs(LOW – fixture maintenance).Including a
"custom-check-runs": []branch in the sharedget_value_side_effectkeeps the cloning tests insulated from the new config key while exercising the same code paths as before. Returning an empty list here is a sensible default for “no custom checks configured” in these scenarios.
649-680: Keepadd_api_users_to_auto_verified...test in sync with new config key (LOW – fixture completeness).The local
get_value_side_effectin this test now also handles"custom-check-runs"by returning[], which matches the rest of the test setup (no custom checks relevant to this behavior) and avoids surprises if_validate_custom_check_runsis invoked during initialization.webhook_server/libs/handlers/issue_comment_handler.py (1)
6-6: AddCoroutineimport to match task typing (LOW – type hygiene).Pulling in
Coroutinefromcollections.abcmatches its use intasks: list[Coroutine[..., ...] | Task[Any]]elsewhere in this handler and keeps type hints explicit without affecting runtime behavior.webhook_server/tests/test_context.py (1)
15-15: LGTM! Comprehensive test coverage for _format_duration helper.The new tests exercise all branches of the _format_duration function (sub-second, seconds, minutes+seconds, exact minutes, hours+minutes, exact hours) and verify the step-without-duration edge case in _build_summary. Test structure follows pytest conventions correctly.
Also applies to: 1071-1114
webhook_server/libs/handlers/check_run_handler.py (2)
216-230: LGTM! Custom check status methods follow established patterns.The four new methods (
set_custom_check_queued,set_custom_check_in_progress,set_custom_check_success,set_custom_check_failure) correctly delegate toset_check_run_statuswith appropriate parameters. The pattern matches existing built-in check methods (tox, pre-commit, etc.) and relies on validated data fromGithubWebhook.custom_check_runs.
447-452: LGTM! Custom checks correctly integrated as required status checks.The loop appends all custom check names to
all_required_status_checks, treating them the same as built-in checks. The comment correctly notes that validation in_validate_custom_check_runsguarantees thenamefield exists, so no defensive checks are needed.webhook_server/libs/github_api.py (3)
625-630: LGTM! Custom check loading integrates cleanly with config system.Loading custom-check-runs from config and validating via
_validate_custom_check_runsis the right pattern. This ensures only valid checks reach the rest of the system. The default empty list maintains backward compatibility.
763-770: LGTM! Custom checks correctly added to retest command support.Appending custom check names to
current_pull_request_supported_retestenables/retest <name>for each custom check. The comment correctly notes validation guarantees the name field exists.
787-858: LGTM! Comprehensive validation with excellent error handling.The
_validate_custom_check_runsmethod implements robust validation:
- Checks for required fields (name, command)
- Prevents collisions with built-in checks (TOX_STR, PRE_COMMIT_STR, etc.)
- Detects duplicate custom check names (first wins)
- Validates executable existence via
shutil.which- Provides clear warnings for invalid entries and summary info logs
The validation is defensive without being paranoid and gives users actionable feedback.
Note: Static analysis flags f-strings in logging (G004), but per project coding guidelines (learning from rnetser), f-strings are the established standard for logging in this codebase.
webhook_server/tests/test_custom_check_runs.py (1)
1-936: LGTM! Comprehensive test coverage for custom check runs feature.This test suite provides excellent coverage across all aspects of the custom check runs feature:
- Schema validation tests (lines 34-81): Verify config structure matches schema expectations
- CheckRunHandler tests (lines 83-193): Test all four status methods and integration with all_required_status_checks
- RunnerHandler tests (lines 195-473): Exercise execution workflow, environment handling, checkout failures, and command execution
- Integration tests (lines 475-541): Verify end-to-end workflow from queue to completion
- Retest tests (lines 543-624): Validate /retest command formatting and triggering
- Validation tests (lines 626-831): Comprehensive coverage of _validate_custom_check_runs logic (missing fields, empty/whitespace commands, executable checks, duplicates, multiline commands)
- Edge cases (lines 833-936): No checks configured, timeouts, long commands
The test structure follows pytest best practices, uses AsyncMock/Mock correctly, and achieves excellent code coverage. The async context manager mocking pattern for
_checkout_worktreeis particularly well done.Note: Static analysis flags
/tmp/...paths as potential security issues (S108), but these are mock strings in tests, not actual temp directory creation, so the warnings are false positives.webhook_server/config/schema.yaml (1)
321-370: Verification confirmed: custom-check-runs examples properly included in test fixture.The
valid_full_configfixture exercises the full schema validation path with two examples: a basic lint check (name + command) and a security-scan check (name + command + env variables). The env field examples (DEBUG=true,SCAN_LEVEL=high) validate the pattern constraint^[^=]+=.+$, confirming that backward compatibility is maintained and the schema aligns with the validation logic in_validate_custom_check_runs.webhook_server/libs/handlers/runner_handler.py (3)
3-8: LGTM!All new imports are used appropriately:
osforos.environ.copy()inrun_custom_checkTask,Callable,Coroutinefor type annotations inrun_retestspartialfor bindingcheck_configtorun_custom_check
486-562: LGTM — clean implementation with proper secret handling.MEDIUM: This method correctly:
- Uses
_checkout_worktreepattern consistent with other runners- Copies
os.environbefore extending (essential sinceenvparam replaces the entire environment)- Collects secret values for redaction and passes them to
run_command- Logs invalid env format as warning (doesn't silently skip)
The secret redaction concern from the previous review has been properly addressed. The only values not redacted would be empty strings (line 535 check), which is intentional since empty values don't need masking.
Note: Ruff G004 warnings about f-string logging are false positives per project guidelines — f-strings are the preferred logging style in this codebase.
676-678: Verify:partialbinding matches method signature.LOW: Just confirming the
partialusage is correct. Whenrunner(pull_request=pull_request)is called on line 687, it expands to:self.run_custom_check(pull_request=pull_request, check_config=custom_check)This matches the
run_custom_check(self, pull_request: PullRequest, check_config: dict[str, Any])signature. The pattern is correct.
| if _supported_retests: | ||
| tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] | ||
| for _test in _supported_retests: | ||
| self.logger.debug(f"{self.log_prefix} running retest {_test}") | ||
| task = asyncio.create_task(_retests_to_func_map[_test](pull_request=pull_request)) | ||
| tasks.append(task) | ||
|
|
||
| results = await asyncio.gather(*tasks, return_exceptions=True) | ||
| for result in results: | ||
| if isinstance(result, Exception): | ||
| self.logger.error(f"{self.log_prefix} Async task failed: {result}") | ||
| # Use runner_handler.run_retests() to avoid duplication | ||
| await self.runner_handler.run_retests( | ||
| supported_retests=_supported_retests, | ||
| pull_request=pull_request, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Centralize retest execution through RunnerHandler.run_retests (MEDIUM – design/duplication).
Delegating supported retests to self.runner_handler.run_retests(...) is a good cleanup:
- It removes the duplicate “test name → runner” dispatch logic from
IssueCommentHandler. - It automatically picks up new custom checks because
run_retestsextends the map withcustom_check_runs. - Existing semantics for unsupported tests and automerge labeling remain controlled in this method (
_supported_retestsis still filtered here before delegation).
This keeps /retest behavior consistent with other entry points and makes future test additions less error‑prone.
🤖 Prompt for AI Agents
In @webhook_server/libs/handlers/issue_comment_handler.py around lines 439 -
444, The duplicate “test name → runner” dispatch in IssueCommentHandler should
be removed and centralized by delegating to RunnerHandler.run_retests; update
the code in IssueCommentHandler (where _supported_retests is computed) to call
await self.runner_handler.run_retests(supported_retests=_supported_retests,
pull_request=pull_request) instead of reimplementing dispatch logic, ensuring
_supported_retests is still filtered prior to the call so unsupported tests
remain excluded and existing automerge/labeling semantics in
RunnerHandler.run_retests are preserved.
| async def run_retests(self, supported_retests: list[str], pull_request: PullRequest) -> None: | ||
| """Run the specified retests for a pull request. | ||
| Args: | ||
| supported_retests: List of test names to run (e.g., ['tox', 'pre-commit']) | ||
| pull_request: The PullRequest object to run tests for | ||
| """ | ||
| if not supported_retests: | ||
| self.logger.debug(f"{self.log_prefix} No retests to run") | ||
| return | ||
|
|
||
| # Map check names to runner functions | ||
| _retests_to_func_map: dict[str, Callable[..., Coroutine[Any, Any, None]]] = { | ||
| TOX_STR: self.run_tox, | ||
| PRE_COMMIT_STR: self.run_pre_commit, | ||
| BUILD_CONTAINER_STR: self.run_build_container, | ||
| PYTHON_MODULE_INSTALL_STR: self.run_install_python_module, | ||
| CONVENTIONAL_TITLE_STR: self.run_conventional_title_check, | ||
| } | ||
|
|
||
| # Add custom check runs to the retest map | ||
| # Note: custom checks are validated in GithubWebhook._validate_custom_check_runs() | ||
| # so name is guaranteed to exist | ||
| for custom_check in self.github_webhook.custom_check_runs: | ||
| check_key = custom_check["name"] | ||
| _retests_to_func_map[check_key] = partial(self.run_custom_check, check_config=custom_check) | ||
|
|
||
| tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] | ||
| for _test in supported_retests: | ||
| runner = _retests_to_func_map.get(_test) | ||
| if runner is None: | ||
| self.logger.error(f"{self.log_prefix} Unknown retest '{_test}' requested, skipping") | ||
| continue | ||
| self.logger.debug(f"{self.log_prefix} running retest {_test}") | ||
| task = asyncio.create_task(runner(pull_request=pull_request)) | ||
| tasks.append(task) | ||
|
|
||
| results = await asyncio.gather(*tasks, return_exceptions=True) | ||
| for result in results: | ||
| if isinstance(result, Exception): | ||
| self.logger.error(f"{self.log_prefix} Async task failed: {result}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — defensive lookup and parallel execution implemented correctly.
MEDIUM: The method properly addresses the past review concern:
- Uses
.get(_test)instead of direct dictionary access (line 682) - Logs error and continues for unknown retests (lines 684-685)
- This is the right balance: fail-safe behavior that still surfaces misconfigurations clearly in logs
The partial(self.run_custom_check, check_config=custom_check) correctly binds the config while leaving pull_request as the remaining required argument.
LOW (optional refinement): The type annotation on line 680 could be simplified since asyncio.create_task() always returns Task[Any]:
- tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = []
+ tasks: list[Task[Any]] = []Not blocking — the current type works correctly.
🧰 Tools
🪛 Ruff (0.14.10)
661-661: Logging statement uses f-string
(G004)
684-684: Logging statement uses f-string
(G004)
686-686: Logging statement uses f-string
(G004)
693-693: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In @webhook_server/libs/handlers/runner_handler.py around lines 653 - 693, The
tasks list is over-annotated; change the type annotation on the local variable
tasks in run_retests to list[Task[Any]] (remove Coroutine[..., Any, Any] union)
because you always call asyncio.create_task which returns Task; update the
declaration of tasks in the run_retests method (refer to the variable name tasks
and the function run_retests) accordingly to simplify the type and keep typing
accurate.
| def test_validate_custom_check_runs_duplicate_names(self, minimal_hook_data: dict, minimal_headers: dict) -> None: | ||
| """Test that duplicate custom check names are rejected.""" | ||
| with patch("webhook_server.libs.github_api.Config") as mock_config: | ||
| mock_config.return_value.repository = True | ||
| mock_config.return_value.repository_local_data.return_value = {} | ||
|
|
||
| # Mock get_value to return custom checks with duplicate names | ||
| def get_value_side_effect(value: str, *_args: object, **_kwargs: object) -> Any: | ||
| if value == "custom-check-runs": | ||
| return [ | ||
| {"name": "my-check", "command": "pytest"}, | ||
| {"name": "my-check", "command": "ruff check"}, # Duplicate name | ||
| {"name": "another-check", "command": "mypy"}, | ||
| ] | ||
| if value == "container": | ||
| return {} | ||
| if value == "pypi": | ||
| return {} | ||
| return None | ||
|
|
||
| mock_config.return_value.get_value.side_effect = get_value_side_effect | ||
|
|
||
| with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: | ||
| mock_get_api.return_value = (Mock(), "token", "apiuser") | ||
|
|
||
| with patch("webhook_server.libs.github_api.get_github_repo_api"): | ||
| with patch("webhook_server.libs.github_api.get_repository_github_app_api"): | ||
| with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix"): | ||
| # Mock shutil.which to return True for all executables | ||
| with patch("shutil.which", return_value="/usr/bin/command"): | ||
| mock_logger = Mock() | ||
| gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) | ||
|
|
||
| # Verify that only unique names are kept (first occurrence wins) | ||
| assert len(gh.custom_check_runs) == 2 | ||
| check_names = [c["name"] for c in gh.custom_check_runs] | ||
| assert "my-check" in check_names | ||
| assert "another-check" in check_names | ||
|
|
||
| # Verify that warning was logged for duplicate | ||
| warning_calls = [str(call) for call in mock_logger.warning.call_args_list] | ||
| assert any("Duplicate custom check name 'my-check'" in call for call in warning_calls) |
There was a problem hiding this comment.
Same get_value_side_effect -> Any issue in duplicate‑names test (HIGH – typing / lint).
This helper at Line 1719 has the same pattern: -> Any while the body only ever returns a list of dicts, {}, or None. To satisfy ANN401 and keep consistency with earlier fixes in this file, narrow the return type accordingly.
For example:
Proposed typed signature
- def get_value_side_effect(value: str, *_args: object, **_kwargs: object) -> Any:
+ def get_value_side_effect(
+ value: str,
+ *_args: object,
+ **_kwargs: object,
+ ) -> list[dict[str, str]] | dict[str, object] | None:The rest of the test logic (collision handling / first‑occurrence‑wins) looks solid.
🧰 Tools
🪛 Ruff (0.14.10)
1719-1719: Dynamically typed expressions (typing.Any) are disallowed in get_value_side_effect
(ANN401)
🤖 Prompt for AI Agents
In @webhook_server/tests/test_github_api.py around lines 1712 - 1753, The local
helper get_value_side_effect in test_validate_custom_check_runs_duplicate_names
currently types its return as Any; narrow it to match actual returns by changing
its annotation to return Optional[Union[List[Dict[str, str]], Dict[str, str]]],
and add the corresponding typing imports (Optional, Union, List, Dict) at the
top of the test file; keep the signature (value: str, *_args: object, **_kwargs:
object) and leave the body unchanged so the test still returns the list of
dicts, {} or None and satisfies ANN401.
- Change get_value_side_effect return type from Any to precise union - test_validate_custom_check_runs_builtin_collision: list[dict[str, Any]] | dict[str, Any] | None - test_validate_custom_check_runs_duplicate_names: list[dict[str, str]] | dict[str, Any] | None Addresses CodeRabbit review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @webhook_server/tests/test_github_api.py:
- Around line 1423-1428: The test helper get_value_side_effect currently types
its return as list[object] | None; tighten this to list[dict[str, object]] |
None to reflect that the "custom-check-runs" config returns a list of
dicts—update the signature of get_value_side_effect accordingly and keep
mock_config.get_value.side_effect = get_value_side_effect unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_github_api.py
🧰 Additional context used
📓 Path-based instructions (2)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_github_api.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_github_api.py
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip with early exit when action != 'completed'
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes `| None` or `Optional`)
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Applied to files:
webhook_server/tests/test_github_api.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_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/tests/test_github_api.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/tests/test_github_api.py
📚 Learning: 2026-01-05T14:43:44.037Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.037Z
Learning: Applies to webhook_server/**/*.py : Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotation `attr: SomeType | None = None`
Applied to files:
webhook_server/tests/test_github_api.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_github_api.py
🧬 Code graph analysis (1)
webhook_server/tests/test_github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
🔇 Additional comments (4)
webhook_server/tests/test_github_api.py (4)
103-104: LGTM: Proper fixture extension for custom check runs.The addition of the empty list return for
"custom-check-runs"correctly extends the fixture to support tests that require this config value. An empty list is the appropriate default for scenarios where no custom checks are configured.
678-679: LGTM: Consistent config mock extension.The addition maintains consistency with the fixture pattern established elsewhere in the test suite. The empty list return properly supports the test setup.
1654-1713: Excellent test coverage for built-in collision validation.This test comprehensively verifies that custom checks with names colliding with built-in checks (tox, pre-commit, build-container, python-module-install, conventional-title) are properly rejected. The test correctly:
- Uses precise type annotations with
objectvarargs and explicit return union- Verifies filtering behavior (only 1 valid check retained from 6 inputs)
- Asserts warning logs for all 5 collision cases
- Aligns with the
_validate_custom_check_runsvalidation logicThe implementation matches project standards and provides valuable coverage for the custom check runs feature.
Based on coding guidelines.
1714-1757: Well-designed test for duplicate name validation.This test effectively verifies that duplicate custom check names are handled correctly with first-occurrence-wins behavior. The test:
- Uses proper type annotations with
objectvarargs and explicit return type- Validates that 2 unique checks are retained from 3 inputs (one duplicate)
- Confirms appropriate warning logging for the duplicate "my-check"
- Follows established test patterns in the file
The implementation aligns with the
_validate_custom_check_runsduplicate-tracking logic and provides essential coverage for this validation rule.Based on coding guidelines.
The feature allows users to define custom checks in their YAML config that:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.