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-configurable custom check runs (name, command, optional mandatory) and validation; exposes generic named check-run APIs; centralizes runner logic to execute built-in and custom checks (including /retest orchestration); updates schema, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub API
participant PR as Pull Request Event
participant PRH as PullRequestHandler
participant CH as CheckRunHandler
participant RH as RunnerHandler
participant WT as Worktree
participant EX as Executor
GH->>PR: emit pull_request event
PR->>PRH: process event (setup)
PRH->>CH: set_check_queued(name="custom-check")
CH->>GH: create check run (queued)
PRH->>RH: run_custom_check(pull_request, CheckConfig)
RH->>CH: set_check_in_progress(name="custom-check")
CH->>GH: update check run (in_progress)
RH->>WT: prepare / checkout PR worktree
RH->>EX: execute command (cwd=worktree, env=merged)
alt command succeeds
RH->>CH: set_check_success(name="custom-check", output)
CH->>GH: update check run (success)
else fails / not found / timeout
RH->>CH: set_check_failure(name="custom-check", output)
CH->>GH: update check run (failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Review notes (severity-guided)CRITICAL (blocking): Ensure custom check names cannot collide with built-in check names. Why: collisions can silently override required checks. Verify HIGH (types/defensive): Confirm MEDIUM (style/consistency): Verify removal of many specific setters from public surface won't break external consumers; ensure docs and any integrations are updated. LOW (suggestion): Document 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webhook_server/libs/handlers/runner_handler.py (1)
429-449: HIGH: Wrappull_request.titleaccess inasyncio.to_thread()to avoid blocking the event loop.PyGithub property accesses are synchronous and can trigger API calls. Line 434 and line 444 access
pull_request.titledirectly in an async method without wrapping, which blocks the event loop. Fetch the title once viaasyncio.to_thread()and reuse it.Suggested fix
- output: CheckRunOutput = { + title = await asyncio.to_thread(lambda: pull_request.title) + output: CheckRunOutput = { "title": "Conventional Title", "summary": "PR title follows Conventional Commits format", "text": ( f"**Format:** `<type>[optional scope]: <description>`\n\n" - f"**Your title:** `{pull_request.title}`\n\n" + f"**Your title:** `{title}`\n\n" f"This title complies with the Conventional Commits v1.0.0 specification." ), } if await self.check_run_handler.is_check_run_in_progress(check_run=CONVENTIONAL_TITLE_STR): self.logger.info(f"{self.log_prefix} Check run is in progress, re-running {CONVENTIONAL_TITLE_STR}.") await self.check_run_handler.set_check_in_progress(name=CONVENTIONAL_TITLE_STR) allowed_names = [name.strip() for name in self.github_webhook.conventional_title.split(",") if name.strip()] - title = pull_request.title + # title already fetched aboveThis violates the coding guideline: "PyGithub is synchronous/blocking — MUST wrap ALL method calls and property accesses with
asyncio.to_thread()to avoid blocking the event loop."webhook_server/tests/test_github_api.py (1)
94-106: Fix return type annotation to include list type.
The function returns[]for"custom-check-runs"but the return type annotation excludes lists. This breaks strict mypy type checking and prevents catching real errors. Per coding guidelines, complete type hints are mandatory in strict mode.Update the return annotation to include the list type:
Proposed fix
- def _get_value_side_effect(value: str, *_args: object, **_kwargs: object) -> bool | dict[str, object] | None: + def _get_value_side_effect( + value: str, *_args: object, **_kwargs: object + ) -> bool | dict[str, object] | list[dict[str, object]] | None:
🤖 Fix all issues with AI agents
In `@webhook_server/tests/test_custom_check_runs.py`:
- Around line 605-723: Replace the manual prefix-slicing assertions with an
integration-style exercise of the real normalization: instantiate
IssueCommentHandler (using the mock_github_webhook and a Mock for its
dependencies), call its process_retest_command (or the public method that
handles comment bodies) with a comment_body like "/retest custom:lint", and
assert that RunnerHandler.run_custom_check (or RunnerHandler.run_custom_check
mocked on the RunnerHandler instance used by the handler) is called with
check_config/name "lint"; this ensures the production normalization logic in
IssueCommentHandler.process_retest_command is exercised rather than duplicating
string slicing in the test.
♻️ Duplicate comments (2)
webhook_server/libs/handlers/runner_handler.py (2)
192-249: CRITICAL: Avoid logging raw commands and fail-closed on unexpected errors.
Line 226 logs the full command before any redaction; custom checks can embed tokens, so this leaks secrets. Also, if an unexpected exception occurs afterset_check_in_progress, the check can remain stuckin_progress. Suppress raw command logging and mark failure on unexpected exceptions.🔧 Suggested hardening
- cmd = check_config.command.replace("{worktree_path}", worktree_path) - self.logger.debug(f"{self.log_prefix} {check_config.name} command to run: {cmd}") + cmd = check_config.command.replace("{worktree_path}", worktree_path) + self.logger.debug( + f"{self.log_prefix} {check_config.name} command prepared (logging suppressed)" + ) # Execute command - use cwd if configured, otherwise command should include paths cwd = worktree_path if check_config.use_cwd else None try: rc, out, err = await run_command( command=cmd, log_prefix=self.log_prefix, mask_sensitive=self.github_webhook.mask_sensitive, cwd=cwd, ) except TimeoutError: self.logger.error(f"{self.log_prefix} Check {check_config.name} timed out") output["text"] = "Command execution timed out" return await self.check_run_handler.set_check_failure(name=check_config.name, output=output) + except Exception as ex: + self.logger.exception( + f"{self.log_prefix} Unexpected failure running {check_config.name}: {ex}" + ) + output["text"] = str(ex) + await self.check_run_handler.set_check_failure(name=check_config.name, output=output) + raise
604-644: HIGH: Propagateasyncio.CancelledErrorand log tracebacks for retest failures.
gather(..., return_exceptions=True)turns cancellations into results and the currentlogger.errordrops tracebacks. Re-raise cancellations and log full exception context to keep shutdown semantics and debugging intact.As per coding guidelines, always re-raise `asyncio.CancelledError`.🔧 Suggested fix
results = await asyncio.gather(*tasks, return_exceptions=True) for result in results: + if isinstance(result, asyncio.CancelledError): + raise result if isinstance(result, Exception): - self.logger.error(f"{self.log_prefix} Async task failed: {result}") + self.logger.exception(f"{self.log_prefix} Async task failed", exc_info=result)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
webhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/conftest.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_github_api.py
🧰 Additional context used
📓 Path-based instructions (5)
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/tests/test_custom_check_runs.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.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.pywebhook_server/tests/conftest.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 (44)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/conftest.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/conftest.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/test_custom_check_runs.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/test_custom_check_runs.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.pywebhook_server/libs/handlers/runner_handler.pywebhook_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/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_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/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/libs/handlers/runner_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_github_api.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2024-10-09T09:16:45.452Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-09T09:16:45.452Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/tests/test_github_api.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/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Store GitHub tokens in environment variables or secret management systems, never commit tokens to repository
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/tests/conftest.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/test_github_api.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/test_github_api.pywebhook_server/tests/conftest.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: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handler classes must implement `__init__(self, github_webhook, ...)` and `process_event(event_data)` pattern
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/tests/conftest.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/tests/test_custom_check_runs.pywebhook_server/tests/conftest.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2025-10-28T16:09:08.689Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely - return types can change (e.g., `Any` → `bool`), method signatures can be modified without deprecation
Applied to files:
webhook_server/tests/test_custom_check_runs.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/conftest.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/tests/conftest.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/conftest.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/conftest.pywebhook_server/libs/github_api.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: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses with `asyncio.to_thread()` to avoid blocking the event loop
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : When accessing PyGithub properties that may trigger API calls, wrap in `asyncio.to_thread(lambda: obj.property)` instead of direct access
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : When iterating PyGithub PaginatedList, wrap iteration in `asyncio.to_thread(lambda: list(...))` to avoid blocking
Applied to files:
webhook_server/libs/handlers/runner_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/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Use `asyncio.gather()` for concurrent PyGithub operations to maximize performance
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Always re-raise `asyncio.CancelledError` after catching it
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Do NOT use defensive checks for architecture guarantees like `repository_data` which is ALWAYS set before handlers instantiate
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-05-10T21:39:34.243Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:281-308
Timestamp: 2025-05-10T21:39:34.243Z
Learning: In the refactored codebase, the `run_podman_command` method in `RunnerHandler` returns a tuple `(bool, str, str)` where the boolean `True` represents success and `False` represents failure, which is the opposite of traditional shell exit codes.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-15T13:18:38.590Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:2053-2062
Timestamp: 2024-10-15T13:18:38.590Z
Learning: In the `run_podman_command` method in `webhook_server_container/libs/github_api.py`, the `rc` variable is a boolean that returns `True` when the command succeeds and `False` when it fails.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-02-25T12:01:42.999Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 666
File: webhook_server_container/libs/github_api.py:2254-2255
Timestamp: 2025-02-25T12:01:42.999Z
Learning: In the ProcessGithubWehook class, the `self.conventional_title` attribute is set during initialization by the `_repo_data_from_config()` method, and doesn't need additional existence checks.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/libs/github_api.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-10-31T13:07:23.714Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/graphql/unified_api.py:333-341
Timestamp: 2025-10-31T13:07:23.714Z
Learning: In this repository (myk-org/github-webhook-server), the maintainer prefers not to clamp or validate user-provided configuration values (e.g., GraphQL query limits). Users should be allowed to set any value they want, and if it causes API failures, they can learn from the error and adjust the configuration themselves. Do not suggest adding validation or clamping logic for config values.
Applied to files:
webhook_server/libs/github_api.py
🧬 Code graph analysis (2)
webhook_server/libs/handlers/runner_handler.py (1)
webhook_server/libs/handlers/check_run_handler.py (6)
CheckRunHandler(39-440)CheckRunOutput(31-36)is_check_run_in_progress(250-256)set_check_in_progress(122-130)set_check_failure(143-152)set_check_success(132-141)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(132-153)
🪛 Ruff (0.14.11)
webhook_server/tests/conftest.py
18-18: Possible hardcoded password assigned to: "TEST_GITHUB_TOKEN"
(S105)
webhook_server/libs/handlers/runner_handler.py
206-206: Logging statement uses f-string
(G004)
208-208: Logging statement uses f-string
(G004)
219-219: Logging statement uses f-string
(G004)
226-226: Logging statement uses f-string
(G004)
238-238: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Logging statement uses f-string
(G004)
245-245: Logging statement uses f-string
(G004)
248-248: Logging statement uses f-string
(G004)
253-253: Logging statement uses f-string
(G004)
274-274: Logging statement uses f-string
(G004)
362-362: Logging statement uses f-string
(G004)
446-446: Logging statement uses f-string
(G004)
612-612: Logging statement uses f-string
(G004)
635-635: Logging statement uses f-string
(G004)
637-637: Logging statement uses f-string
(G004)
644-644: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
900-901: Logging statement uses f-string
(G004)
926-926: Logging statement uses f-string
(G004)
938-938: Logging statement uses f-string
(G004)
944-944: Logging statement uses f-string
(G004)
949-949: Logging statement uses f-string
(G004)
954-954: Logging statement uses f-string
(G004)
961-961: Logging statement uses f-string
(G004)
967-968: Logging statement uses f-string
(G004)
973-973: Logging statement uses f-string
(G004)
980-980: Logging statement uses f-string
(G004)
986-986: Logging statement uses f-string
(G004)
992-994: Logging statement uses f-string
(G004)
1001-1001: Logging statement uses f-string
(G004)
1005-1005: Logging statement uses f-string
(G004)
1007-1007: Logging statement uses f-string
(G004)
🔇 Additional comments (21)
webhook_server/tests/conftest.py (1)
17-18: LOW: Centralized TEST_GITHUB_TOKEN is a good test hook.
Single source of truth keeps mocks consistent across tests; the allowlist pragma is appropriate for a fake token. Based on learnings, this matches the test-mocking convention.webhook_server/libs/github_api.py (3)
657-662: LOW: Custom check runs are loaded and validated in the right place.
Centralizing validation during config load keeps downstream handlers deterministic.
855-861: LOW: Custom checks are correctly included in supported retests.
This keeps/retestdiscovery aligned with validated configuration.
878-1009: LOW: Validation pipeline is robust and user-friendly.
Type guards, safe-name regex, duplicate detection, shlex parsing, and executable checks cover common misconfigs without crashing.webhook_server/libs/handlers/runner_handler.py (4)
35-50: LOW: CheckConfig provides a clean, typed contract for check execution.
slots+frozen reduce accidental mutation and keep the runner API consistent.
251-280: LOW: Unified CheckConfig usage is consistent for tox/pre-commit/module install.
This keeps lifecycle logic centralized and reduces duplicated check handling.Also applies to: 421-423
309-364: LOW: Container check status updates align with the generic check API.
TheCheckRunOutputwiring and status transitions are consistent with the new pattern.
488-512: The schema does not supportenvas a separate field for custom checks.The schema at
webhook_server/config/schema.yamllines 408–424 defines custom-check-runs items withadditionalProperties: false, which explicitly forbids fields beyondname,command, andmandatory. The schema intentionally requires environment variables to be embedded directly in the command string (as shown in the examples:TOKEN=xyz DEBUG=true uv tool run --from bandit bandit -r .), not as a separateenvfield.The test fixture in
test_config_schema.pyline 86 includes an"env"field, but:
- This violates the schema constraint (
additionalProperties: false)- The
Configclass does not validate against the schema—it only loads YAML- The
envfield is silently ignored, not processedNo code changes are needed. Environment variables should remain embedded in the command string as the schema design intends.
webhook_server/tests/test_github_api.py (5)
17-17: LOW: Centralized test-token usage looks good.
This keeps token usage consistent and avoids scattered literals.
679-680: LOW: Config stub now covers custom checks.
Good to include"custom-check-runs"in the mocked config to keep the test aligned with new behavior.
1496-1501: LOW: No additional feedback on this block.
1980-2042: LOW: Collision validation test is solid.
Covers all built-in collisions and asserts the warning messages clearly.
2043-2090: LOW: Duplicate-name validation test is solid.
Good coverage of “first occurrence wins” behavior and warning emission.webhook_server/tests/test_custom_check_runs.py (8)
1-33: LOW: Clear module docstring and focused imports.
Helps future maintainers understand the scope and intent quickly.
36-151: LOW: Validation coverage is comprehensive.
These tests exercise both happy-path and invalid-config cases with realistic command parsing.
153-269: LOW: CheckRunHandler custom-check status tests look good.
Covers queued, in-progress, success/failure, and required-status aggregation.
271-382: LOW: Mandatory/optional behavior is well covered.
Nice separation of “required for merge” vs “still executed” logic.
384-531: LOW: RunnerHandler custom-check execution tests look good.
The worktree handling and status transitions are thoroughly exercised.
533-603: LOW: Integration workflow test is clear and focused.
Good coverage of the end-to-end custom check flow.
725-1031: LOW: Validation logic edge cases are well covered.
Great breadth across naming rules, whitespace commands, and executable detection.
1033-1147: LOW: Edge-case coverage looks solid.
Timeout handling and long-command cases are exercised clearly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix security leak in run_check() by removing command debug log - Handle CancelledError properly in run_retests() - Move strip() to command source for efficiency - Add shared BUILTIN_CHECK_NAMES constant - Update schema example to use uv - Improve test coverage and reuse fixtures
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/libs/handlers/runner_handler.py (1)
443-463: Wrappull_request.titleaccess inasyncio.to_thread()to avoid blocking the event loop.PyGithub property access is synchronous and can trigger network I/O. In async handlers, this blocks the event loop. Extract the title once before constructing the output dict and reuse the cached value:
title = await asyncio.to_thread(lambda: pull_request.title) output: CheckRunOutput = { "title": "Conventional Title", "summary": "PR title follows Conventional Commits format", "text": ( f"**Format:** `<type>[optional scope]: <description>`\n\n" f"**Your title:** `{title}`\n\n" f"This title complies with the Conventional Commits v1.0.0 specification." ), }This also eliminates the redundant property access at line 458.
🤖 Fix all issues with AI agents
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 655-663: The retest logging misattributes failures because it
indexes supported_retests by results order; instead build and use an explicit
list of scheduled test names aligned with tasks (e.g., create scheduled_tests =
[] when you append each task to tasks) so you can reference scheduled_tests[idx]
when reporting errors; update the block using asyncio.gather(*tasks,
return_exceptions=True) to iterate results and use scheduled_tests (not
supported_retests) for the test_name in the logger calls (keep handling of
asyncio.CancelledError and re-raising unchanged).
♻️ Duplicate comments (1)
webhook_server/config/schema.yaml (1)
394-400: MEDIUM: Remove token-like example fromcustom-check-runsdocs.
Config files are often committed; showingTOKEN=xyznormalizes storing secrets in plaintext and risks accidental leakage even with masking. Use a non-secret placeholder or omit the env assignment in examples. Based on learnings, avoid token env examples because parallel commands can collide and secrets shouldn’t be embedded in config.💡 Suggested doc fix
- - name: security-scan - command: TOKEN=xyz DEBUG=true uv tool run --from bandit bandit -r . + - name: security-scan + command: DEBUG=true uv tool run --from bandit bandit -r .
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_github_api.pywebhook_server/utils/constants.pywebhook_server/utils/github_repository_settings.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_github_api.pywebhook_server/utils/constants.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/utils/github_repository_settings.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.pywebhook_server/tests/test_config_schema.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/**/*.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 (46)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_config_schema.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/config/schema.yamlwebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_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/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_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/tests/test_github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_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/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2024-10-09T09:16:45.452Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-09T09:16:45.452Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/tests/test_github_api.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/tests/test_github_api.pywebhook_server/config/schema.yamlwebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_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/tests/test_github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Store GitHub tokens in environment variables or secret management systems, never commit tokens to repository
Applied to files:
webhook_server/tests/test_github_api.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/tests/test_github_api.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/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: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handler classes must implement `__init__(self, github_webhook, ...)` and `process_event(event_data)` pattern
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.pywebhook_server/utils/constants.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_config_schema.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2025-10-28T16:09:08.689Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely - return types can change (e.g., `Any` → `bool`), method signatures can be modified without deprecation
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.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: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses with `asyncio.to_thread()` to avoid blocking the event loop
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : When accessing PyGithub properties that may trigger API calls, wrap in `asyncio.to_thread(lambda: obj.property)` instead of direct access
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : When iterating PyGithub PaginatedList, wrap iteration in `asyncio.to_thread(lambda: list(...))` to avoid blocking
Applied to files:
webhook_server/libs/handlers/runner_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/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Use `asyncio.gather()` for concurrent PyGithub operations to maximize performance
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Always re-raise `asyncio.CancelledError` after catching it
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Do NOT use defensive checks for architecture guarantees like `repository_data` which is ALWAYS set before handlers instantiate
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/libs/handlers/runner_handler.py
📚 Learning: 2025-05-10T21:39:34.243Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:281-308
Timestamp: 2025-05-10T21:39:34.243Z
Learning: In the refactored codebase, the `run_podman_command` method in `RunnerHandler` returns a tuple `(bool, str, str)` where the boolean `True` represents success and `False` represents failure, which is the opposite of traditional shell exit codes.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-15T13:18:38.590Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:2053-2062
Timestamp: 2024-10-15T13:18:38.590Z
Learning: In the `run_podman_command` method in `webhook_server_container/libs/github_api.py`, the `rc` variable is a boolean that returns `True` when the command succeeds and `False` when it fails.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-02-25T12:01:42.999Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 666
File: webhook_server_container/libs/github_api.py:2254-2255
Timestamp: 2025-02-25T12:01:42.999Z
Learning: In the ProcessGithubWehook class, the `self.conventional_title` attribute is set during initialization by the `_repo_data_from_config()` method, and doesn't need additional existence checks.
Applied to files:
webhook_server/libs/handlers/runner_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
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/libs/github_api.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-10-31T13:07:23.714Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/graphql/unified_api.py:333-341
Timestamp: 2025-10-31T13:07:23.714Z
Learning: In this repository (myk-org/github-webhook-server), the maintainer prefers not to clamp or validate user-provided configuration values (e.g., GraphQL query limits). Users should be allowed to set any value they want, and if it causes API failures, they can learn from the error and adjust the configuration themselves. Do not suggest adding validation or clamping logic for config values.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Use hasattr() checks only for platform constants that may not exist on all platforms (e.g., `os.O_NOFOLLOW`)
Applied to files:
webhook_server/libs/github_api.pywebhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : Do NOT use hasattr() checks for required parameters in `__init__()` - if config is required, access it directly without defensive checks
Applied to files:
webhook_server/utils/github_repository_settings.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/**/*.py : All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Applied to files:
webhook_server/utils/github_repository_settings.py
🧬 Code graph analysis (2)
webhook_server/tests/test_custom_check_runs.py (3)
webhook_server/libs/github_api.py (1)
_validate_custom_check_runs(879-1003)webhook_server/libs/handlers/check_run_handler.py (6)
CheckRunHandler(39-440)set_check_queued(111-120)set_check_in_progress(122-130)set_check_success(132-141)set_check_failure(143-152)all_required_status_checks(355-390)webhook_server/libs/handlers/runner_handler.py (2)
RunnerHandler(53-663)run_custom_check(502-526)
webhook_server/libs/handlers/runner_handler.py (1)
webhook_server/libs/handlers/check_run_handler.py (6)
CheckRunOutput(31-36)is_check_run_in_progress(250-256)set_check_in_progress(122-130)get_check_run_text(192-248)set_check_failure(143-152)set_check_success(132-141)
🪛 Ruff (0.14.11)
webhook_server/libs/handlers/runner_handler.py
207-207: Logging statement uses f-string
(G004)
209-209: Logging statement uses f-string
(G004)
220-220: Logging statement uses f-string
(G004)
239-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
239-239: Logging statement uses f-string
(G004)
246-246: Logging statement uses f-string
(G004)
249-249: Logging statement uses f-string
(G004)
253-253: Logging statement uses f-string
(G004)
256-256: Logging statement uses f-string
(G004)
267-267: Logging statement uses f-string
(G004)
288-288: Logging statement uses f-string
(G004)
376-376: Logging statement uses f-string
(G004)
460-460: Logging statement uses f-string
(G004)
626-626: Logging statement uses f-string
(G004)
649-649: Logging statement uses f-string
(G004)
651-651: Logging statement uses f-string
(G004)
658-658: Logging statement uses f-string
(G004)
663-663: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
901-902: Logging statement uses f-string
(G004)
917-917: Logging statement uses f-string
(G004)
929-929: Logging statement uses f-string
(G004)
935-935: Logging statement uses f-string
(G004)
940-940: Logging statement uses f-string
(G004)
945-945: Logging statement uses f-string
(G004)
952-952: Logging statement uses f-string
(G004)
958-959: Logging statement uses f-string
(G004)
967-967: Logging statement uses f-string
(G004)
974-974: Logging statement uses f-string
(G004)
980-980: Logging statement uses f-string
(G004)
986-988: Logging statement uses f-string
(G004)
995-995: Logging statement uses f-string
(G004)
999-999: Logging statement uses f-string
(G004)
1001-1001: Logging statement uses f-string
(G004)
🔇 Additional comments (26)
webhook_server/tests/test_config_schema.py (2)
81-88: LOW: Good coverage ofcustom-check-runsin the full config fixture.
This keeps the schema change exercised end-to-end in config loading. As per coding guidelines, schema changes should be tested.
142-153: LOW: Assertions forcustom-check-runsstructure look solid.
Validates required fields and the default behavior formandatory.webhook_server/utils/constants.py (1)
129-137: LOW: CentralizedBUILTIN_CHECK_NAMESimproves consistency.
Single source of truth makes collision checks and settings updates safer.webhook_server/utils/github_repository_settings.py (1)
19-27: LOW: UsingBUILTIN_CHECK_NAMESand a frozenset signature is clean and consistent.
Reduces duplication and keeps built-in checks aligned across call sites.Also applies to: 334-361
webhook_server/tests/test_github_api.py (5)
17-17: LOW: UsingTEST_GITHUB_TOKENaligns with test token guidelines.
Keeps token usage centralized and consistent. As per coding guidelines.
94-106: LOW:custom-check-runshandled in config side-effect mock.
Prevents unrelated clone tests from breaking on the new config key.
650-681: LOW:custom-check-runscoverage added to the add‑api‑users config mock.
Keeps the mock aligned with the new config path.
1478-1497: LOW: Reusing the sharedget_value_side_effectkeeps mocks consistent.
Avoids brittle tests as Config keys evolve.
1975-2085: LOW: Strong tests for built‑in collisions and duplicate custom checks.
Validates filtering behavior and warning logs without touching real executables.webhook_server/libs/github_api.py (3)
658-663: LOW: LGTM — custom checks are loaded via config and validated up front.
Good use of a centralized validation hook before wiring into runtime flows.
855-862: LOW: LGTM — custom checks are appended to supported retest targets.
Keeps/retestbehavior aligned with repo config.
879-1003: LOW: LGTM — validator is thorough (names, duplicates, env-prefixes, executables).
The guardrails and summary logging make misconfigurations easy to diagnose.webhook_server/tests/test_custom_check_runs.py (8)
36-151: LOW: LGTM — validation tests cover happy path + missing fields.
Clear assertions for required keys and command parsing behavior.
153-270: LOW: LGTM — CheckRunHandler custom check status methods are exercised well.
Good coverage of queued/in_progress/success/failure semantics.
271-383: LOW: LGTM — mandatory flag behavior and queueing rules are covered.
Nice separation of “required for merge” vs “executed for all.”
384-531: LOW: LGTM — runner flow tests cover success, failure, checkout, and cwd usage.
These cases map directly to the unified run_check lifecycle.
533-604: LOW: LGTM — integration flow validates end-to-end custom check execution.
Verifies the expected in_progress → success sequence.
605-701: LOW: LGTM — retest command/name semantics match config expectations.
Confirms raw names are used without prefixes.
702-1009: LOW: LGTM — validator edge cases (unsafe names, missing executables, env-prefix) are solid.
Good breadth for regression coverage.
1010-1124: LOW: LGTM — edge-case behavior for “no checks” and timeouts is covered.
Ensures graceful failure paths remain tested.webhook_server/libs/handlers/runner_handler.py (6)
35-50: LOW: LGTM — CheckConfig cleanly encapsulates per-check metadata.
Makes the unified run_check API straightforward to reuse.
192-263: LOW: LGTM — unified run_check flow is solid with timeout + error handling.
Good lifecycle sequencing and cancellation safety.
270-294: LOW: LGTM — built-in checks now delegate consistently through CheckConfig.
This reduces drift between built-ins and custom checks.Also applies to: 435-437
323-378: LOW: LGTM — container build now uses unified check status updates.
Keeps status reporting consistent with other checks.
502-526: LOW: LGTM — custom checks wrap into unified run_check with cwd execution.
Keeps validation centralized in GithubWebhook.
542-613: LOW: LGTM — cherry-pick check status updates are wired consistently.
Matches the unified check-run reporting style.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-961 published |
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-961 published |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webhook_server/libs/handlers/runner_handler.py (2)
444-464: HIGH: Wrappull_request.titleaccess inasyncio.to_thread()to prevent blocking the event loop.PyGithub properties are synchronous and can trigger API calls. Direct access to
pull_request.titlein the async method blocks the event loop. Per repo guidelines, fetch the value once and reuse it.🔧 Suggested fix
- output: CheckRunOutput = { + title = await asyncio.to_thread(lambda: pull_request.title) + output: CheckRunOutput = { "title": "Conventional Title", "summary": "PR title follows Conventional Commits format", "text": ( f"**Format:** `<type>[optional scope]: <description>`\n\n" - f"**Your title:** `{pull_request.title}`\n\n" + f"**Your title:** `{title}`\n\n" f"This title complies with the Conventional Commits v1.0.0 specification." ), } @@ - title = pull_request.title + # `title` already fetched via asyncio.to_thread above
539-604: Wrap unprotected PyGithub property accesses inasyncio.to_thread()to prevent event-loop blocking.Lines 543, 551–553, and 602 access
pull_requestproperties directly without wrapping. These synchronous PyGithub operations block the event loop. Fetch all properties once viaasyncio.gather()and reuse the values, especially at lines 551–553 where multiple properties are accessed sequentially.🔧 Suggested fix
- new_branch_name = f"{CHERRY_PICKED_LABEL}-{pull_request.head.ref}-{shortuuid.uuid()[:5]}" + head_ref = await asyncio.to_thread(lambda: pull_request.head.ref) + new_branch_name = f"{CHERRY_PICKED_LABEL}-{head_ref}-{shortuuid.uuid()[:5]}" @@ - commit_hash = pull_request.merge_commit_sha - commit_msg_striped = pull_request.title.replace("'", "") - pull_request_url = pull_request.html_url + commit_hash, pr_title, pull_request_url = await asyncio.gather( + asyncio.to_thread(lambda: pull_request.merge_commit_sha), + asyncio.to_thread(lambda: pull_request.title), + asyncio.to_thread(lambda: pull_request.html_url), + ) + commit_msg_striped = pr_title.replace("'", "") @@ - local_branch_name = f"{pull_request.head.ref}-{target_branch}" + local_branch_name = f"{head_ref}-{target_branch}"
|
/verified
|
|
rebase so we can merge it @rnetser |
…add-plugin-option
|
/verified |
|
/lgtm |
|
Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-961. |
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
3 participants
The feature allows users to define custom checks in their YAML config that:
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.