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 per-repository Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub API
participant PR as Pull Request Event
participant PRHandler as PullRequest Handler
participant CheckHandler as CheckRun Handler
participant Runner as Runner Handler
participant Worktree as Repo Worktree
participant Exec as Command Executor
GH->>PR: pull_request.opened / synchronized
PR->>PRHandler: process_opened_or_synchronized
PRHandler->>CheckHandler: set_check_queued(name=custom-check)
CheckHandler->>GH: create check run (queued)
PRHandler->>Runner: run_custom_check(pull_request, check_config)
Runner->>CheckHandler: set_check_in_progress(name=custom-check)
CheckHandler->>GH: update check run (in_progress)
Runner->>Worktree: prepare/checkout PR worktree
Runner->>Exec: execute command (cwd=worktree, env=merged)
alt command succeeds
Runner->>CheckHandler: set_check_success(name=custom-check, output)
CheckHandler->>GH: update check run (success)
else command fails / not found / timeout
Runner->>CheckHandler: set_check_failure(name=custom-check, output)
CheckHandler->>GH: update check run (failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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.
| # Shell metacharacters and operators that could be used for injection | ||
| DANGEROUS_SHELL_PATTERNS: list[tuple[str, str]] = [ | ||
| (r"[;&|]", "Shell operators (;, &, |) are not allowed"), | ||
| (r"\$\(", "Command substitution $() is not allowed"), | ||
| (r"`", "Backtick command substitution is not allowed"), | ||
| (r"\$\{", "Variable expansion ${} is not allowed"), | ||
| (r"\$[A-Za-z_]", "Variable expansion $VAR is not allowed"), | ||
| (r"[><]", "Redirections (>, <) are not allowed"), | ||
| (r"\|\|", "Logical OR (||) is not allowed"), | ||
| (r"&&", "Logical AND (&&) is not allowed"), | ||
| (r"\\n|\\r", "Newline escapes are not allowed"), | ||
| (r"\beval\b", "eval command is not allowed"), | ||
| (r"\bexec\b", "exec command is not allowed"), | ||
| (r"\bsource\b", "source command is not allowed"), | ||
| (r"\bsh\b", "sh command is not allowed"), | ||
| (r"\bbash\b", "bash command is not allowed"), | ||
| (r"\bzsh\b", "zsh command is not allowed"), | ||
| (r"\bcurl\b", "curl command is not allowed"), | ||
| (r"\bwget\b", "wget command is not allowed"), | ||
| (r"\bnc\b|\bnetcat\b", "netcat/nc command is not allowed"), | ||
| (r"\brm\s+-rf", "rm -rf is not allowed"), | ||
| (r"\bsudo\b", "sudo is not allowed"), | ||
| (r"\bsu\b", "su command is not allowed"), | ||
| (r"\bchmod\b", "chmod is not allowed"), | ||
| (r"\bchown\b", "chown is not allowed"), | ||
| (r"\bmkdir\s+-p\s+/", "Creating directories in root is not allowed"), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Consider pre-compiling regex patterns for performance.
These patterns are currently compiled on every call to validate_command_security. For a defense-in-depth module that may be called frequently, pre-compilation would reduce overhead.
Additionally, the pattern r"[;&|]" on line 22 will match a single & character, which could create false positives for commands with URLs containing query parameters (e.g., --url "https://example.com?a=1&b=2"). However, given the security-first approach and that uv tool run commands shouldn't typically need such URLs, this is acceptable.
🔎 Optional: Pre-compile patterns for better performance
import re
from typing import NamedTuple
# Pre-compile patterns for performance
_DANGEROUS_SHELL_PATTERNS: list[tuple[re.Pattern[str], str]] = [
(re.compile(r"[;&|]", re.IGNORECASE), "Shell operators (;, &, |) are not allowed"),
# ... remaining patterns
]🤖 Prompt for AI Agents
In webhook_server/utils/command_security.py around lines 20 to 46, the list
currently stores raw regex strings which are compiled repeatedly at runtime;
replace it with a module-level pre-compiled list (use re.compile for each
pattern, e.g. re.compile(r"...", re.IGNORECASE) as appropriate) and update the
type annotation to list[tuple[re.Pattern[str], str]]; also add the required
import for re at the top. Ensure any code that iterates this list
(validate_command_security) uses the compiled patterns directly (calling
pattern.search/match) rather than re.compile again.
| # Sensitive paths that should never be accessed | ||
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | ||
| (r"/etc/", "Access to /etc/ is not allowed"), | ||
| (r"/root/", "Access to /root/ is not allowed"), | ||
| (r"~/.ssh", "Access to SSH keys is not allowed"), | ||
| (r"/proc/", "Access to /proc/ is not allowed"), | ||
| (r"/sys/", "Access to /sys/ is not allowed"), | ||
| (r"/dev/", "Access to /dev/ is not allowed"), | ||
| (r"/var/log/", "Access to /var/log/ is not allowed"), | ||
| (r"/boot/", "Access to /boot/ is not allowed"), | ||
| (r"\.\.\/", "Path traversal (..) is not allowed"), | ||
| (r"\.env", "Access to .env files is not allowed"), | ||
| (r"config\.yaml", "Access to config.yaml is not allowed"), | ||
| (r"credentials", "Access to credentials files is not allowed"), | ||
| (r"\.pem\b", "Access to PEM files is not allowed"), | ||
| (r"\.key\b", "Access to key files is not allowed"), | ||
| (r"id_rsa", "Access to SSH private keys is not allowed"), | ||
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Path traversal pattern only catches forward slashes.
Line 58's pattern r"\.\.\/ catches ../ but not ..\\ (Windows-style) or standalone .. at command end. While this server likely runs on Linux, defense-in-depth should consider both.
Also, line 52's ~/.ssh works correctly but the tilde isn't escaped—it's benign here since ~ only has special meaning inside character classes [~], but escaping it as \~ would be more explicit.
🔎 Proposed enhancement for path traversal detection
- (r"\.\.\/", "Path traversal (..) is not allowed"),
+ (r"\.\.[/\\]", "Path traversal (..) is not allowed"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Sensitive paths that should never be accessed | |
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | |
| (r"/etc/", "Access to /etc/ is not allowed"), | |
| (r"/root/", "Access to /root/ is not allowed"), | |
| (r"~/.ssh", "Access to SSH keys is not allowed"), | |
| (r"/proc/", "Access to /proc/ is not allowed"), | |
| (r"/sys/", "Access to /sys/ is not allowed"), | |
| (r"/dev/", "Access to /dev/ is not allowed"), | |
| (r"/var/log/", "Access to /var/log/ is not allowed"), | |
| (r"/boot/", "Access to /boot/ is not allowed"), | |
| (r"\.\.\/", "Path traversal (..) is not allowed"), | |
| (r"\.env", "Access to .env files is not allowed"), | |
| (r"config\.yaml", "Access to config.yaml is not allowed"), | |
| (r"credentials", "Access to credentials files is not allowed"), | |
| (r"\.pem\b", "Access to PEM files is not allowed"), | |
| (r"\.key\b", "Access to key files is not allowed"), | |
| (r"id_rsa", "Access to SSH private keys is not allowed"), | |
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | |
| ] | |
| # Sensitive paths that should never be accessed | |
| SENSITIVE_PATH_PATTERNS: list[tuple[str, str]] = [ | |
| (r"/etc/", "Access to /etc/ is not allowed"), | |
| (r"/root/", "Access to /root/ is not allowed"), | |
| (r"~/.ssh", "Access to SSH keys is not allowed"), | |
| (r"/proc/", "Access to /proc/ is not allowed"), | |
| (r"/sys/", "Access to /sys/ is not allowed"), | |
| (r"/dev/", "Access to /dev/ is not allowed"), | |
| (r"/var/log/", "Access to /var/log/ is not allowed"), | |
| (r"/boot/", "Access to /boot/ is not allowed"), | |
| (r"\.\.[/\\]", "Path traversal (..) is not allowed"), | |
| (r"\.env", "Access to .env files is not allowed"), | |
| (r"config\.yaml", "Access to config.yaml is not allowed"), | |
| (r"credentials", "Access to credentials files is not allowed"), | |
| (r"\.pem\b", "Access to PEM files is not allowed"), | |
| (r"\.key\b", "Access to key files is not allowed"), | |
| (r"id_rsa", "Access to SSH private keys is not allowed"), | |
| (r"id_ed25519", "Access to SSH private keys is not allowed"), | |
| ] |
🤖 Prompt for AI Agents
In webhook_server/utils/command_security.py around lines 48 to 66, the
path-traversal regex r"\.\.\/" only matches "../" (forward slash) and the
"~/.ssh" pattern leaves tilde unescaped; update the traversal detection to also
match Windows-style backslashes and standalone/terminal ".." instances (e.g.,
patterns that match "..\", ".." at end or by itself) and escape the tilde in the
SSH pattern for clarity; keep patterns anchored or use alternation to cover
"../", "..\", and trailing ".." cases while preserving existing error messages.
Remove complex security validation layer and adopt a trust-based approach: - Delete command_security.py and related security validation logic - Simplify schema to only name, command, env fields - Add shutil.which() check to gracefully skip unavailable commands - Add set_custom_check_skipped() for handling missing commands - Update tests to reflect simplified validation model This change assumes users will configure safe commands and gracefully handles cases where commands are not found instead of blocking them.
Simplify env configuration by specifying only variable names instead of key-value pairs. Values are read from server environment at runtime. Changes: - Schema: env changed from object to array of strings - Handler: reads env var values from os.environ - Tests: updated for new env list format
Added example showing environment variable reference in custom-check-runs commands to improve documentation clarity.
Remove special handling for custom checks - they now fail naturally like built-in checks (tox, pre-commit, etc.) when commands are not found. Changes: - runner_handler.py: Removed skip logic for missing commands - check_run_handler.py: Removed set_custom_check_skipped method - schema.yaml: Simplified description to reflect new behavior - test_custom_check_runs.py: Removed skip/trigger/timeout tests
Allows env entries in custom check run configuration to use two formats:
- Variable name only (VAR) - reads from server environment
- Variable with explicit value (VAR=value) - uses provided value
This enables mixed usage like:
env:
- PYTHONPATH
- DEBUG=true
Custom checks now behave exactly like built-in checks (tox, pre-commit): - Run on all PR events (no trigger filtering) - Always required for merge (no required field)
- Validate name, command, and executable existence at config load time - Remove duplicate validation from 5 locations - Remove configurable timeout (use default like built-in checks) - Check if command executable exists on server before accepting check
- Use functools.partial instead of nested function in run_retests() - Add custom_check_runs to mock fixtures in test files
Clean up remnants from old logging patterns not removed during structured logging migration. Replaces logger.step (undefined) and format_task_fields (undefined) with standard logger.info calls. Affected handlers: - pull_request_handler.py: setup tasks and CI/CD tasks logging - runner_handler.py: custom check execution logging
…add-plugin-option
- Remove "variable name only" env var option, require explicit VAR=value format - Consolidate command validation in _validate_custom_check_runs - Remove redundant COMPLETED_STR from custom check success/failure methods - Add tests for validation edge cases and duration formatting - Achieve 90% test coverage
When passing env to asyncio.create_subprocess_exec(), it REPLACES the entire environment instead of extending it. Without os.environ.copy(), the subprocess wouldn't have PATH, HOME, or other essential variables, causing commands like 'uv', 'python', etc. to fail. - Change env_dict from empty dict to os.environ.copy() - Add explanatory comment for future maintainers - Update tests to verify environment inheritance
…add-plugin-option
Consolidate check run methods and improve code quality: 1. Replace 24+ specific check run methods with 4 generic methods: - set_check_queued() - generic queued status - set_check_in_progress() - generic in-progress status - set_check_success() - generic success with output - set_check_failure() - generic failure with output 2. Optimize .strip() in github_api.py: - Strip command once at start, reuse stripped variable - Eliminates redundant string operation 3. Enhance missing executable warning message: - Add guidance to open issue or submit PR for missing executables - Improves contributor experience 4. Update all callers in runner_handler.py and pull_request_handler.py: - Use generic methods instead of specific ones - Add conditional checks before queuing (only if feature configured) 5. Update all tests: - test_check_run_handler.py - generic method tests - test_runner_handler.py - updated method calls - test_pull_request_handler.py - updated method calls - test_custom_check_runs.py - updated method calls All changes maintain backward compatibility and pass existing test coverage.
Add optional 'mandatory' boolean field to custom check runs configuration to control whether a check must pass for PR mergeability. Changes: - Schema: added 'mandatory' property (default: true for backward compatibility) - Check run handler: only mandatory checks added to required status checks - Pull request handler: welcome message shows '(optional)' for non-mandatory checks - Tests: 4 new tests verifying mandatory option behavior Backward compatible: checks without 'mandatory' field default to true.
Simplify custom check run configuration by removing the separate 'env' field. Users can now specify environment variables directly inline in the command using standard shell syntax (e.g., 'ENV_VAR=value command'). Changes: - Schema: Removed 'env' property from custom_check_runs configuration - Schema: Updated description to document inline env var syntax - Runner handler: Removed env parameter and handling from run_custom_check() - Tests: Removed 5 env-related test cases (148 lines) This simplification reduces configuration complexity while maintaining full functionality through standard shell syntax.
…add-plugin-option
Remove 9 legacy methods (set_verify_check_*, set_merge_check_*, set_cherry_pick_*) and replace with generic set_check_queued, set_check_in_progress, set_check_success, and set_check_failure methods. Update all handler files and tests to use the new generic pattern.
…heck flow - Add CheckConfig dataclass for unified check configuration - Create unified run_check() method for all command-based checks - Refactor run_tox(), run_pre_commit(), run_install_python_module() to use run_check() - Refactor run_custom_check() to use run_check()
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
webhook_server/libs/handlers/issue_comment_handler.py (1)
391-446: MAJOR:/retest custom:<name>format requires prefix normalization—custom checks won't execute.The code expects normalized test names (e.g.,
'lint') incurrent_pull_request_supported_retest, but users typing/retest custom:lintwill have_target_tests = ['custom:lint']after the split. The membership check at line 425 fails because'custom:lint'is not in the list containing'lint', causing custom checks to be added to_not_supported_retestsinstead of_supported_retests.Strip the
custom:prefix from test names before the membership check:for _test in _target_tests: test_name = _test.replace('custom:', '', 1) if _test.startswith('custom:') else _test if test_name in self.github_webhook.current_pull_request_supported_retest: _supported_retests.append(test_name) else: _not_supported_retests.append(_test)webhook_server/tests/test_pull_request_handler.py (1)
84-116: MEDIUM: Remove duplicated patches ofset_check_queued(only last patch applies).Lines 1661-1664 and 1667 (and similarly 1691-1693) patch the same
pull_request_handler.check_run_handler.set_check_queuedmultiple times in one context manager tuple. That’s easy to misread and can mask wrong expectations.Recommend: patch it once and assert call counts / args on that single AsyncMock.
Also applies to: 1659-1673, 1689-1704
webhook_server/libs/handlers/check_run_handler.py (1)
146-177: CRITICAL: Fix cancellation handling and GitHub Checks API invariants; avoid logging secrets.
Cancellation semantics:
except Exceptioncatchesasyncio.CancelledErrorand prevents proper task cancellation (can hang shutdown/retries). Per coding guidelines, re-raise it.Checks API invariants: When you send
conclusion, explicitly setstatus="completed"alongside it. The GitHub API auto-completes the status, but the code should be explicit. Currently, the exception path sendsconclusion=FAILURE_STRwithout ensuringstatus="completed".Secret leakage:
self.logger.debug(f"{self.log_prefix} Set check run status with {kwargs}")dumps the entirekwargsdict, which may contain sensitive command output/secrets. Log only non-sensitive fields (name, status, conclusion, has_output flag).Suggested fix
try: - self.logger.debug(f"{self.log_prefix} Set check run status with {kwargs}") + self.logger.debug( + f"{self.log_prefix} Set check run status" + f" name={check_run} status={kwargs.get('status', '')} conclusion={kwargs.get('conclusion', '')}" + f" has_output={'output' in kwargs}" + ) await asyncio.to_thread(self.github_webhook.repository_by_github_app.create_check_run, **kwargs) if conclusion in (SUCCESS_STR, IN_PROGRESS_STR): self.logger.info(msg) return +except asyncio.CancelledError: + raise except Exception as ex: - self.logger.debug(f"{self.log_prefix} Failed to set {check_run} check to {status or conclusion}, {ex}") + self.logger.exception(f"{self.log_prefix} Failed to set {check_run} check to {status or conclusion}") + kwargs["status"] = "completed" kwargs["conclusion"] = FAILURE_STR await asyncio.to_thread(self.github_webhook.repository_by_github_app.create_check_run, **kwargs)When setting
conclusion, ensurestatus="completed"is always present.
🤖 Fix all issues with AI agents
In @webhook_server/config/schema.yaml:
- Around line 321-358: The schema allows inline env vars in custom-check-runs ->
items -> properties.command, so update the runtime validator/executor that
parses and executes these commands to (1) correctly parse out leading KEY=VALUE
environment assignments to determine the real executable (so
resolution/validation uses the actual command token, not the env prefix) and (2)
never log or persist the raw command string or any captured env values; instead
log only a sanitized representation (e.g., executable name and non-secret args)
and pass env assignments directly to the process execution API as environment
variables without including them in telemetry or error messages. Ensure the
command parsing and execution path that handles custom-check-runs honors the
schema's default mandatory flag and rejects additionalProperties as specified.
In @webhook_server/libs/handlers/check_run_handler.py:
- Around line 361-368: The loop that appends custom check names to
all_required_status_checks can create duplicates when later concatenated with
branch_required_status_checks; after collecting both lists (symbols:
branch_required_status_checks and all_required_status_checks, and the
custom_check_runs loop), deduplicate while preserving order before further use
or logging (e.g., replace the final concatenation usage with an ordered-unique
list via list(dict.fromkeys(...)) or a simple seen-set filter) so check names
are reported only once across branch protection, built-ins, and custom checks.
- Around line 103-145: The set_check_* wrappers (set_check_queued,
set_check_in_progress, set_check_success, set_check_failure) accept output:
dict[str, Any] which can allow non-JSON-serializable values; tighten this by
defining a specific output TypedDict (e.g., CheckOutput =
TypedDict('CheckOutput', {'title': str, 'summary': str, 'text': Optional[str]},
total=False)) or a narrow Mapping[str, str], and add a small
validation/serialization step inside set_check_run_status (or just before
calling create_check_run) that ensures only allowed keys and JSON-serializable
values are passed (convert non-str types or raise a clear ValueError); update
the wrappers to use the new TypedDict/typing and call the validator to prevent
runtime failures from PyGithub.
In @webhook_server/libs/handlers/pull_request_handler.py:
- Around line 360-366: Add strict name validation in
GithubWebhook._validate_custom_check_runs() to reject unsafe characters for
custom_check_runs: ensure each custom_check["name"] matches a whitelist regex
like alphanumerics, dot, underscore, hyphen and length limits (e.g.,
^[a-zA-Z0-9._-]{1,64}$); if a name fails, log a warning and skip it so the
downstream loop that builds retest_msg (the for custom_check in
self.github_webhook.custom_check_runs ... retest_msg += ... using check_name)
never receives unsafe values that could break markdown rendering.
- Around line 633-651: The setup code currently unconditionally queues built-in
checks (TOX_STR, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, BUILD_CONTAINER_STR
and CONVENTIONAL_TITLE_STR) via check_run_handler.set_check_queued which leaves
them indefinitely queued when their runners return early for disabled features;
update the logic so either (A) only append set_check_queued for each check when
the corresponding feature flag on self.github_webhook (or other feature gate
used by the runner) is true, or (B) when each runner early-returns (the methods
that handle tox, pre-commit, python module install, build container), call
check_run_handler.set_check_success (or equivalent) with a “skipped/not
configured” output for TOX_STR, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR,
BUILD_CONTAINER_STR (and CONVENTIONAL_TITLE_STR when conventional_title is
false) so queued checks reach a terminal state; apply the same conditional
queuing or terminal-skipping behavior for custom_check_runs in the
github_webhook.custom_check_runs loop as well.
In @webhook_server/libs/handlers/runner_handler.py:
- Around line 439-440: The regex check currently constructs a list inside any()
which causes unnecessary allocation; replace the list comprehension with a
generator expression so the pattern matching is short-circuited and memory is
not allocated for all results — update the conditional that iterates over
allowed_names (the expression using re.match and title) to use a generator
expression, leaving the subsequent call to
self.check_run_handler.set_check_success(name=CONVENTIONAL_TITLE_STR,
output=output) unchanged.
- Around line 245-263: In run_tox, avoid direct PyGithub property access to
pull_request.base.ref; call that property inside asyncio.to_thread and await it
(e.g., read base_ref = await asyncio.to_thread(lambda: pull_request.base.ref))
and then use base_ref to look up _tox_tests and build the cmd; update references
in this function so all PyGithub accesses (pull_request.base.ref) are performed
via asyncio.to_thread to prevent blocking the event loop.
- Around line 595-635: run_retests currently calls asyncio.gather(...,
return_exceptions=True) and then logs exceptions only via str(result), which
swallows tracebacks and treats asyncio.CancelledError like a normal exception;
change the logic to (1) call asyncio.gather without return_exceptions or handle
results but explicitly detect asyncio.CancelledError and re-raise it so
cancellations propagate, and (2) when logging other exceptions from the gather
results or awaited tasks in run_retests, log full tracebacks (e.g., use
logger.exception or format_exception) instead of just the exception string;
update references in this function (_retests_to_func_map, asyncio.create_task,
asyncio.gather, and the final results loop) to implement those two changes.
- Around line 192-234: The code currently uses
check_config.command.format(worktree_path=worktree_path) which will raise
KeyError for any other braces in user-provided commands; change that to a
targeted replacement such as cmd =
check_config.command.replace("{worktree_path}", worktree_path) (or an equivalent
safe substitution) in the run_check method so only the {worktree_path} token is
replaced and arbitrary braces in the command string are preserved, leaving the
subsequent run_command call and cwd logic unchanged.
In @webhook_server/tests/test_custom_check_runs.py:
- Around line 608-805: The validator and tests are missing handling for commands
that begin with environment variable assignments (e.g., "TOKEN=xyz uv ...") so
GithubWebhook._validate_custom_check_runs currently calls shutil.which on the
first token ("TOKEN=xyz") and rejects the check; update the validator to strip
any leading env-var assignments (tokens matching /^[A-Za-z_][A-Za-z0-9_]*=.*/),
optionally repeated, before extracting the executable token (respecting
quoted/path tokens), then call shutil.which on that executable; add a unit test
similar to other cases that supplies a raw check like {"name": "env-prefixed",
"command": "TOKEN=xyz PATH=/x/bin echo hi"} and assert it validates and logs
appropriately.
- Around line 1-73: The schema tests in TestCustomCheckRunsSchemaValidation only
assert Python dict literals; update test_valid_custom_check_config,
test_minimal_custom_check_config and test_custom_check_with_multiline_command to
call the real production schema validator (use the project’s config/schema
validation function or existing schema test helper) with the fixtures
valid_custom_check_config and minimal_custom_check_config (and the multiline
config) and assert that validation succeeds (no exception) and/or returns the
expected validated structure; if a shared validator or schema fixture already
exists in the test suite, import and reuse it instead of asserting raw dict
contents.
- Around line 513-595: The test uses "/retest custom:<name>" but
IssueCommentHandler.process_retest_command() matches against
current_pull_request_supported_retest which contains raw names (e.g. "lint"), so
normalize the incoming command in IssueCommentHandler.process_retest_command():
when parsing a "/retest" argument strip any "custom:" prefix (or equivalently
map supported names to "custom:<name>" before matching) so that names like
"custom:lint" match the existing list; update the parsing branch that extracts
retest_name and ensure RunnerHandler.run_custom_check and any callers (e.g.,
current_pull_request_supported_retest population logic) continue to use raw
check names internally.
- Around line 305-316: The test fixture mock_github_webhook sets
mock_webhook.clone_repo_dir to a hardcoded "/tmp/test-repo"; replace this with a
portable temporary directory (use the pytest tmp_path fixture or
tempfile.TemporaryDirectory) and assign mock_webhook.clone_repo_dir to that path
(as a str or Path) so tests are portable and avoid Ruff S108; make the same
change for the other fixtures/tests that hardcode /tmp/... (e.g., the other mock
webhook fixtures and tests mentioned) so all uses use tmp_path/tempfile.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
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_custom_check_runs.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_runner_handler.py
🧰 Additional context used
📓 Path-based instructions (7)
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/issue_comment_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/libs/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/libs/github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/runner_handler.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/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/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_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/tests/test_pull_request_handler.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain backward compatibility for user-facing configuration files (
config.yaml,.github-webhook-server.yaml) - configuration schema changes must support old formats or provide migration
Files:
webhook_server/config/schema.yaml
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/config/schema.yaml: Mask sensitive data in logs by default (setmask-sensitive-data: truein configuration)
Configuration schema changes should be tested withuv run pytest webhook_server/tests/test_config_schema.py -v
Files:
webhook_server/config/schema.yaml
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
🧠 Learnings (26)
📓 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: 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_issue_comment_handler.pywebhook_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/issue_comment_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/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/libs/github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.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/libs/handlers/pull_request_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_runner_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/runner_handler.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_pull_request_handler.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 with early exit when action != 'completed'
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_runner_handler.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/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_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/tests/test_pull_request_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/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_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/tests/test_pull_request_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/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_issue_comment_handler.pywebhook_server/config/schema.yamlwebhook_server/tests/test_check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/tests/test_pull_request_handler.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_issue_comment_handler.pywebhook_server/tests/test_check_run_handler.pywebhook_server/tests/test_runner_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_issue_comment_handler.pywebhook_server/tests/test_check_run_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 **/*.{yaml,yml} : Maintain backward compatibility for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 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/config/schema.yamlwebhook_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/tests/test_check_run_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_check_run_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_runner_handler.pywebhook_server/tests/test_pull_request_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 : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/tests/test_check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/check_run_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 : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/tests/test_check_run_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.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/tests/test_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/tests/test_runner_handler.pywebhook_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
🧬 Code graph analysis (8)
webhook_server/libs/handlers/issue_comment_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (2)
set_check_queued(103-112)set_check_success(124-133)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(595-635)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (4)
set_check_success(124-133)set_check_queued(103-112)set_check_in_progress(114-122)set_check_failure(135-144)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(479-503)
webhook_server/tests/test_issue_comment_handler.py (2)
webhook_server/tests/test_custom_check_runs.py (1)
check_run_handler(96-98)webhook_server/tests/test_check_run_handler.py (1)
check_run_handler(54-56)
webhook_server/tests/test_check_run_handler.py (1)
webhook_server/libs/handlers/check_run_handler.py (4)
set_check_queued(103-112)set_check_success(124-133)set_check_in_progress(114-122)set_check_failure(135-144)
webhook_server/tests/test_custom_check_runs.py (2)
webhook_server/libs/github_api.py (2)
GithubWebhook(78-877)_validate_custom_check_runs(787-861)webhook_server/libs/handlers/check_run_handler.py (1)
all_required_status_checks(338-372)
webhook_server/tests/test_runner_handler.py (2)
webhook_server/libs/handlers/runner_handler.py (2)
CheckConfig(36-50)run_check(192-243)webhook_server/libs/handlers/check_run_handler.py (5)
is_check_run_in_progress(233-239)set_check_in_progress(114-122)set_check_success(124-133)set_check_failure(135-144)get_check_run_text(178-231)
webhook_server/tests/test_pull_request_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (4)
set_check_queued(103-112)set_check_in_progress(114-122)set_check_success(124-133)set_check_failure(135-144)webhook_server/libs/handlers/pull_request_handler.py (1)
_process_verified_for_update_or_new_pull_request(904-933)
webhook_server/libs/handlers/runner_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (4)
is_check_run_in_progress(233-239)set_check_in_progress(114-122)set_check_failure(135-144)set_check_success(124-133)webhook_server/utils/helpers.py (1)
run_command(266-417)
🪛 Ruff (0.14.10)
webhook_server/libs/handlers/pull_request_handler.py
652-652: Logging statement uses f-string
(G004)
686-686: Logging statement uses f-string
(G004)
930-930: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
823-823: Logging statement uses f-string
(G004)
828-828: Logging statement uses f-string
(G004)
835-835: Logging statement uses f-string
(G004)
845-847: Logging statement uses f-string
(G004)
853-853: Logging statement uses f-string
(G004)
857-857: Logging statement uses f-string
(G004)
859-859: Logging statement uses f-string
(G004)
webhook_server/tests/test_custom_check_runs.py
313-313: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
348-348: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
377-377: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
425-425: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
441-441: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
458-458: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
495-495: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
570-570: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
815-815: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
864-864: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
899-899: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
webhook_server/tests/test_runner_handler.py
174-174: Probable insecure usage of temporary file or directory: "/tmp/worktree-path"
(S108)
1069-1069: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1084-1084: Probable insecure usage of temporary file or directory: "/tmp/test-repo"
(S108)
1086-1086: Possible hardcoded password assigned to: "token"
(S105)
1122-1122: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1151-1151: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1195-1195: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1210-1210: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1224-1224: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
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)
225-225: Logging statement uses f-string
(G004)
239-239: Logging statement uses f-string
(G004)
242-242: Logging statement uses f-string
(G004)
247-247: Logging statement uses f-string
(G004)
266-266: Logging statement uses f-string
(G004)
354-354: Logging statement uses f-string
(G004)
438-438: Logging statement uses f-string
(G004)
439-439: Unnecessary list comprehension
Remove unnecessary comprehension
(C419)
603-603: Logging statement uses f-string
(G004)
626-626: Logging statement uses f-string
(G004)
628-628: Logging statement uses f-string
(G004)
635-635: Logging statement uses f-string
(G004)
🔇 Additional comments (9)
webhook_server/tests/test_issue_comment_handler.py (2)
28-45: LOW: Good test fixture alignment (addscustom_check_runs).
This keeps themock_github_webhookfixture compatible with the new custom-check plumbing (avoids attribute errors when handlers touchGithubWebhook.custom_check_runs).
487-529: LOW: Verified label now uses generic check APIs (correct assertions).
Both the “add” and “cancel” paths now validateset_check_success(name=VERIFIED_LABEL_STR)/set_check_queued(name=VERIFIED_LABEL_STR), matching the refactor.webhook_server/libs/handlers/issue_comment_handler.py (2)
5-7: LOW: Typing import change is fine.
No behavior change; just ensure the task list staysTask[...]-only if you want tighter types later.
279-286: LOW: Verified check now correctly uses genericset_check_*APIs.
This keeps “verified” consistent with custom/built-in checks via the unified CheckRunHandler surface.webhook_server/tests/test_check_run_handler.py (2)
29-51: LOW: Fixture update addscustom_check_runs(keeps handler construction stable).
118-370: LOW: Solid unit coverage for the new genericset_check_*wrappers.
These tests lock in the intended mapping from convenience methods →set_check_run_status(...), including the newoutput: dict | Nonecontract.webhook_server/libs/github_api.py (2)
625-630: LOW: Loadingcustom-check-runsin repo config is correctly wired into the webhook object.
This makes the feature available to handlers via a single source of truth (GithubWebhook.custom_check_runs).
763-770: MEDIUM: Supported retest names are raw custom check names—ensure this matches/retestUX.
If you keep raw names here, then/retest <name>should be the documented format (notcustom:<name>), or you need to add normalization.webhook_server/tests/test_runner_handler.py (1)
46-47: MEDIUM: Align tests with repo conventions (AsyncMock assertions + TEST_GITHUB_TOKEN).
- Prefer
assert_awaited_once_with(...)when the patched target is awaited (several places still useassert_called_once_with).- Tokens are hardcoded (e.g., Line 30 / Line 1086). Repo guideline says to use
TEST_GITHUB_TOKEN.Also applies to: 1038-1238
⛔ Skipped due to 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/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokensLearnt 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`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 CILearnt 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'
| mandatory: false | ||
| - name: complex-check | ||
| command: | | ||
| python -c " |
| seen_names: set[str] = set() | ||
|
|
||
| # Built-in check names that custom checks cannot override | ||
| BUILTIN_CHECK_NAMES = { |
There was a problem hiding this comment.
we have this in another place https://github.com/myk-org/github-webhook-server/blob/main/webhook_server/utils/github_repository_settings.py#L335
reuse please
- Fix env-var command parsing and stop logging secrets (github_api.py) - Add CheckRunOutput TypedDict for type safety (check_run_handler.py) - Deduplicate required status checks (check_run_handler.py) - Fix cancellation handling with explicit status=completed (check_run_handler.py) - Add strict name validation for custom checks (github_api.py) - Conditional check queuing for disabled features (pull_request_handler.py) - Fix command substitution to use replace() instead of format() (runner_handler.py) - Wrap PyGithub access in asyncio.to_thread (runner_handler.py) - Use generator expression instead of list in any() (runner_handler.py) - Fix run_retests exception handling and CancelledError propagation (runner_handler.py) - Fix /retest custom: prefix normalization (issue_comment_handler.py) - Use tmp_path instead of hardcoded /tmp in tests - Add tests for env-var-prefixed commands - Update schema validation tests to use real validator - Remove duplicated patches in tests
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webhook_server/libs/handlers/issue_comment_handler.py (1)
399-420: CRITICAL:/retest alldetection uses substring match and will mis-route commands (e.g./retest install).
"all" in command_argsis true for many strings; you want token-level detection after normalization. Also consider filtering empty tokens after strippingcustom:.Proposed fix
_target_tests: list[str] = command_args.split() # Strip "custom:" prefix if present (users may type "/retest custom:lint" but we expect "lint") -_target_tests = [t[7:] if t.startswith("custom:") else t for t in _target_tests] +prefix = "custom:" +_target_tests = [t[len(prefix) :] if t.startswith(prefix) else t for t in _target_tests] +_target_tests = [t for t in _target_tests if t] # drop "custom:" → "" edge case ... -if "all" in command_args: +if "all" in _target_tests:webhook_server/libs/handlers/runner_handler.py (1)
424-445: HIGH: Avoid directpull_request.titleaccess in async path (PyGithub can block the loop).
Project rule is “wrap ALL PyGithub property accesses withasyncio.to_thread”.Proposed fix
- output: CheckRunOutput = { + pr_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:** `{pr_title}`\n\n" f"This title complies with the Conventional Commits v1.0.0 specification." ), } ... - title = pull_request.title + title = pr_titleAlso applies to: 481-482
🤖 Fix all issues with AI agents
In @webhook_server/libs/handlers/pull_request_handler.py:
- Around line 360-366: Update the retest message generation where it iterates
self.github_webhook.custom_check_runs (the block that builds retest_msg using
check_name and status_indicator) to append an extra note for each custom check
indicating that the handler also accepts the normalized form `/retest
custom:<name>`; e.g., for each check include both `/retest {check_name}` and
`/retest custom:{check_name}` (or a parenthetical mentioning the alternate form)
so users see both accepted syntaxes.
In @webhook_server/libs/handlers/runner_handler.py:
- Around line 599-639: In run_retests, the post-gather error handling must
re-raise asyncio.CancelledError instead of treating it as a regular Exception;
change the results loop that currently does "for result in results: if
isinstance(result, Exception): ..." to explicitly detect and re-raise
asyncio.CancelledError (and any other BaseException that isn’t an Exception)
while continuing to log normal Exception instances via self.logger.error;
reference the results variable and the run_retests method and ensure you check
isinstance(result, asyncio.CancelledError) first, then if isinstance(result,
Exception) log it, else re-raise the result.
In @webhook_server/tests/test_custom_check_runs.py:
- Around line 386-397: The fixture mock_github_webhook uses tmp_path annotated
as Any which fails ANN401; change the parameter type to pathlib.Path (i.e.,
tmp_path: Path) and add the corresponding from pathlib import Path import at the
top of the test module; update the same pattern in the other fixtures mentioned
(the blocks around lines 529-552 and 1019-1032) so all tmp_path parameters are
typed as Path to avoid leaking Any.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
webhook_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_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.py
🧰 Additional context used
📓 Path-based instructions (5)
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.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.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_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 (35)
📓 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: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/handlers/pull_request_handler.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.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 : 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.pywebhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_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/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.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/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/pull_request_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.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_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/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_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/handlers/pull_request_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/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 : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_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: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/check_run_handler.pywebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/issue_comment_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/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_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/libs/handlers/issue_comment_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/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_custom_check_runs.pywebhook_server/tests/test_pull_request_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 : 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_custom_check_runs.pywebhook_server/tests/test_pull_request_handler.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/tests/test_custom_check_runs.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/libs/handlers/check_run_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 : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.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: 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/libs/github_api.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
🧬 Code graph analysis (4)
webhook_server/tests/test_custom_check_runs.py (2)
webhook_server/libs/github_api.py (1)
_validate_custom_check_runs(788-884)webhook_server/libs/handlers/check_run_handler.py (1)
CheckRunOutput(31-36)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
webhook_server/libs/handlers/pull_request_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (5)
CheckRunOutput(31-36)set_check_success(132-141)set_check_queued(111-120)set_check_in_progress(122-130)set_check_failure(143-152)webhook_server/libs/handlers/runner_handler.py (1)
run_custom_check(483-507)
webhook_server/libs/handlers/issue_comment_handler.py (2)
webhook_server/libs/handlers/check_run_handler.py (2)
set_check_queued(111-120)set_check_success(132-141)webhook_server/libs/handlers/runner_handler.py (1)
run_retests(599-639)
🪛 Ruff (0.14.10)
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)
240-240: Logging statement uses f-string
(G004)
243-243: Logging statement uses f-string
(G004)
248-248: Logging statement uses f-string
(G004)
269-269: Logging statement uses f-string
(G004)
357-357: Logging statement uses f-string
(G004)
441-441: Logging statement uses f-string
(G004)
607-607: Logging statement uses f-string
(G004)
630-630: Logging statement uses f-string
(G004)
632-632: Logging statement uses f-string
(G004)
639-639: Logging statement uses f-string
(G004)
webhook_server/tests/test_custom_check_runs.py
387-387: Dynamically typed expressions (typing.Any) are disallowed in tmp_path
(ANN401)
429-429: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
458-458: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
506-506: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
522-522: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
529-529: Dynamically typed expressions (typing.Any) are disallowed in tmp_path
(ANN401)
576-576: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
674-674: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1023-1023: Dynamically typed expressions (typing.Any) are disallowed in tmp_path
(ANN401)
1077-1077: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
1112-1112: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
webhook_server/libs/handlers/check_run_handler.py
177-177: Logging statement uses f-string
(G004)
182-182: Consider moving this statement to an else block
(TRY300)
187-187: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
831-831: Logging statement uses f-string
(G004)
836-836: Logging statement uses f-string
(G004)
841-841: Logging statement uses f-string
(G004)
848-848: Logging statement uses f-string
(G004)
855-855: Logging statement uses f-string
(G004)
861-861: Logging statement uses f-string
(G004)
867-869: Logging statement uses f-string
(G004)
876-876: Logging statement uses f-string
(G004)
880-880: Logging statement uses f-string
(G004)
882-882: Logging statement uses f-string
(G004)
webhook_server/libs/handlers/pull_request_handler.py
662-662: Logging statement uses f-string
(G004)
696-696: Logging statement uses f-string
(G004)
940-940: Logging statement uses f-string
(G004)
🔇 Additional comments (23)
webhook_server/tests/test_pull_request_handler.py (5)
84-84: LGTM!Adding
custom_check_runs = []to the mock correctly initializes the new attribute thatPullRequestHandlernow expects fromGithubWebhook. This aligns with the custom check runs feature addition.
113-116: LGTM!The mock methods are correctly updated to use the new generic API names (
set_check_queued,set_check_in_progress,set_check_success,set_check_failure). UsingAsyncMockis correct since these are async methods in the production code.
324-327: LGTM!The patch targets and assertions are correctly updated to use the new unified API (
set_check_success,set_check_queued) with the requirednameparameter. The assertions properly verify the check name is passed as a keyword argument.Also applies to: 338-341
617-622: LGTM!All verified processing tests are consistently updated to use the new generic check-run API with proper
name=VERIFIED_LABEL_STRassertions.Also applies to: 632-637, 653-658, 674-679
735-735: LGTM!The patches for
set_check_in_progressandset_check_queuedare correctly updated to target the new generic method names oncheck_run_handler.Also applies to: 1661-1661, 1688-1688
webhook_server/libs/handlers/check_run_handler.py (6)
31-37: LGTM! Good use of TypedDict for type safety.The
CheckRunOutputTypedDict provides clear typing for the check run output parameter. Usingtotal=Falsemakes all fields optional by default, andNotRequired[str | None]fortextcorrectly indicates it's both optional and nullable.
111-152: LGTM! Clean API consolidation.The four generic methods (
set_check_queued,set_check_in_progress,set_check_success,set_check_failure) effectively replace the previous per-check setters. This reduces code duplication and provides a unified interface for both built-in and custom checks. The docstrings clearly document the purpose and parameters.
166-168: Good fix: Explicit status when conclusion is provided.Setting
status="completed"explicitly when a conclusion is provided ensures the GitHub Check Runs API receives consistent state. This aligns with the commit message mentioning "corrected cancellation to explicitly set status=completed."
184-190: LGTM! Correct CancelledError handling.Re-raising
asyncio.CancelledErrorimmediately is the correct pattern per the coding guidelines: "Always re-raiseasyncio.CancelledErrorafter catching it." The exception handling also properly setsstatus="completed"alongsideconclusion=FAILURE_STRfor consistency.
383-384: Ordered deduplication preserves priority correctly.Using
dict.fromkeys()for ordered deduplication is a good pattern. Branch-required checks come first, followed by config checks, ensuring branch protection settings take precedence in the ordering.
375-384: The code correctly assumescustom_checkhas a guaranteednamekey.GithubWebhook._validate_custom_check_runs()filters out all checks with missing, unsafe, or duplicate names—only validated checks are stored inself.custom_check_runs. The direct accesscustom_check["name"]is safe and appropriate. Theget("mandatory", True)default for backward compatibility is sensible.webhook_server/libs/handlers/issue_comment_handler.py (3)
6-6: LOW: Type import/use is fine for concurrent command execution tasks.
Keepstaskstyping accurate when mixingCoroutineandTask.Also applies to: 92-106
279-286: LOW: VERIFIED label now uses generic check-run setters consistently.
This matches the newCheckRunHandler.set_check_{queued,success}API and reduces special-casing.
441-446: LOW: Delegating toRunnerHandler.run_retests()is a good de-duplication.
Just ensurerun_retests()preserves cancellation semantics (see review onRunnerHandler.run_retests).webhook_server/tests/test_custom_check_runs.py (1)
1-16: LOW: Test coverage breadth is strong (schema + handler + runner + integration).
This should materially protect the new custom-check-runs surface from regressions.Also applies to: 35-381
webhook_server/libs/github_api.py (3)
626-631: LOW: Wiring config →self.custom_check_runsvia a single validator is clean.
This keeps downstream code simple (it can assume validated entries).
764-771: LOW: Adding custom checks tocurrent_pull_request_supported_retestis the right UX for/retest.
Names are propagated without prefixes, matching the handler normalization.
788-884: The original review comment mischaracterizes the feature's design. The maintainer has intentionally chosen a permissive approach for custom check execution, as evidenced by:
- Schema documentation explicitly allows arbitrary commands (
python -c, plainuv, echo, etc.), not justuv tool run- No "uv tool run only" policy exists — the validator checks that the executable exists via
shutil.which()and that command syntax is valid, but does not restrict which executable can be used- Prior CodeRabbit review (commit b27635e) already addressed custom checks and deliberately omitted "uv tool run" enforcement
- Maintainer preference (from repository learnings): configuration values should be validated only for safety (syntax, name collisions), not clamped or restricted to specific patterns. Users are expected to set values as they need and debug failures themselves
The validator appropriately validates:
- Name field: safe characters (alphanumeric, dots, underscores, hyphens), no collisions with built-in checks
- Command field: valid shell quoting, non-empty
- Executable: exists on server (via
shutil.which())This is sufficient for the feature's design goal: allow users to define and run custom commands in their repository worktree. The arbitrary executables are by design, not an oversight.
Likely an incorrect or invalid review comment.
webhook_server/libs/handlers/runner_handler.py (2)
35-51: LOW:CheckConfigis a solid abstraction for unifying built-in and custom checks.
Small surface area, immutable, and makesrun_check()predictable.
483-508: LOW:run_custom_check()correctly reusesrun_check()withuse_cwd=True.
This keeps custom checks consistent with built-ins (status lifecycle + worktree).webhook_server/libs/handlers/pull_request_handler.py (3)
12-12: LOW: Genericset_check_{queued,success}usage for VERIFIED is consistent across label and auto-verify flows.
Reduces bespoke API surface area and aligns tests/handlers.Also applies to: 191-195, 928-944
633-705: LOW: Queueing only enabled built-ins + all custom checks is a good correctness/perf tradeoff.
This prevents “queued forever” checks for disabled features and keeps custom checks first-class.
977-1069: LOW:CheckRunOutputtyping + generic set_check_{in_progress,success,failure} makes merge-check output handling clearer.
Nice alignment withCheckRunHandler’s new API and the custom-check required-status logic.
| # Add custom check runs (both mandatory and optional) | ||
| for custom_check in self.github_webhook.custom_check_runs: | ||
| check_name = custom_check["name"] | ||
| is_mandatory = custom_check.get("mandatory", True) | ||
| status_indicator = "" if is_mandatory else " (optional)" | ||
| retest_msg += f" * `/retest {check_name}` - {check_name}{status_indicator}\n" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LOW: Welcome /retest list correctly includes custom checks (and marks optional).
Consider also documenting that /retest custom:<name> is accepted (since the handler normalizes it).
🤖 Prompt for AI Agents
In @webhook_server/libs/handlers/pull_request_handler.py around lines 360 - 366,
Update the retest message generation where it iterates
self.github_webhook.custom_check_runs (the block that builds retest_msg using
check_name and status_indicator) to append an extra note for each custom check
indicating that the handler also accepts the normalized form `/retest
custom:<name>`; e.g., for each check include both `/retest {check_name}` and
`/retest custom:{check_name}` (or a parenthetical mentioning the alternate form)
so users see both accepted syntaxes.
| async def run_check(self, pull_request: PullRequest, check_config: CheckConfig) -> None: | ||
| """Unified check execution method for both built-in and custom checks. | ||
| if await self.check_run_handler.is_check_run_in_progress(check_run=TOX_STR): | ||
| self.logger.debug(f"{self.log_prefix} Check run is in progress, re-running {TOX_STR}.") | ||
| This method handles the common lifecycle for all command-based checks: | ||
| 1. Set check to in_progress | ||
| 2. Checkout worktree | ||
| 3. Execute command | ||
| 4. Report success or failure | ||
| python_ver = ( | ||
| f"--python={self.github_webhook.tox_python_version}" if self.github_webhook.tox_python_version else "" | ||
| ) | ||
| _tox_tests = self.github_webhook.tox.get(pull_request.base.ref, "") | ||
| Args: | ||
| pull_request: The pull request to run the check on. | ||
| check_config: Configuration for the check (name, command, title, use_cwd). | ||
| """ | ||
| if await self.check_run_handler.is_check_run_in_progress(check_run=check_config.name): | ||
| self.logger.debug(f"{self.log_prefix} Check run is in progress, re-running {check_config.name}.") | ||
|
|
||
| await self.check_run_handler.set_run_tox_check_in_progress() | ||
| self.logger.info(f"{self.log_prefix} Starting check: {check_config.name}") | ||
| await self.check_run_handler.set_check_in_progress(name=check_config.name) | ||
|
|
||
| async with self._checkout_worktree(pull_request=pull_request) as (success, worktree_path, out, err): | ||
| # Build tox command with worktree path | ||
| cmd = f"uvx {python_ver} {TOX_STR} --workdir {worktree_path} --root {worktree_path} -c {worktree_path}" | ||
| if _tox_tests and _tox_tests != "all": | ||
| tests = _tox_tests.replace(" ", "") | ||
| cmd += f" -e {tests}" | ||
| self.logger.debug(f"{self.log_prefix} Tox command to run: {cmd}") | ||
|
|
||
| output: dict[str, Any] = { | ||
| "title": "Tox", | ||
| output: CheckRunOutput = { | ||
| "title": check_config.title, | ||
| "summary": "", | ||
| "text": None, | ||
| } | ||
|
|
||
| if not success: | ||
| self.logger.error(f"{self.log_prefix} Repository preparation failed for tox") | ||
| self.logger.error(f"{self.log_prefix} Repository preparation failed for {check_config.name}") | ||
| output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) | ||
| return await self.check_run_handler.set_run_tox_check_failure(output=output) | ||
| return await self.check_run_handler.set_check_failure(name=check_config.name, output=output) | ||
|
|
||
| # Build command with worktree path substitution | ||
| # Use replace() instead of format() to avoid KeyError on other braces in user commands | ||
| cmd = check_config.command.replace("{worktree_path}", worktree_path) | ||
| self.logger.debug(f"{self.log_prefix} {check_config.name} command to run: {cmd}") | ||
|
|
||
| # Execute command - use cwd if configured, otherwise command should include paths | ||
| cwd = worktree_path if check_config.use_cwd else None | ||
| rc, out, err = await run_command( | ||
| command=cmd, | ||
| log_prefix=self.log_prefix, | ||
| mask_sensitive=self.github_webhook.mask_sensitive, | ||
| cwd=cwd, | ||
| ) | ||
|
|
||
| output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out) | ||
|
|
||
| if rc: | ||
| return await self.check_run_handler.set_run_tox_check_success(output=output) | ||
| self.logger.info(f"{self.log_prefix} Check {check_config.name} completed successfully") | ||
| return await self.check_run_handler.set_check_success(name=check_config.name, output=output) | ||
| else: | ||
| return await self.check_run_handler.set_run_tox_check_failure(output=output) | ||
| self.logger.info(f"{self.log_prefix} Check {check_config.name} failed") | ||
| return await self.check_run_handler.set_check_failure(name=check_config.name, output=output) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the runner_handler.py file to see the actual code
find . -name "runner_handler.py" -type fRepository: myk-org/github-webhook-server
Length of output: 121
🏁 Script executed:
# Look for the run_command function definition
rg -n "def run_command" --type pyRepository: myk-org/github-webhook-server
Length of output: 131
🏁 Script executed:
# Check the mask_sensitive implementation
rg -n "mask_sensitive" webhook_server/libs/handlers/runner_handler.py -B 2 -A 2Repository: myk-org/github-webhook-server
Length of output: 2267
🏁 Script executed:
# Read the run_command function implementation
sed -n '266,350p' webhook_server/utils/helpers.pyRepository: myk-org/github-webhook-server
Length of output: 4130
🏁 Script executed:
# Read the actual runner_handler.py lines 192-245 to see the current code
sed -n '192,245p' webhook_server/libs/handlers/runner_handler.pyRepository: myk-org/github-webhook-server
Length of output: 2873
🏁 Script executed:
# Check how mask_sensitive is actually used in logging
rg -n "mask_sensitive" webhook_server/utils/helpers.py -B 2 -A 2 | head -40Repository: myk-org/github-webhook-server
Length of output: 1609
🏁 Script executed:
# Search for exception handling patterns in similar handler methods
rg -n "except (asyncio.CancelledError|Exception)" webhook_server/libs/handlers/ -A 3 | head -50Repository: myk-org/github-webhook-server
Length of output: 4687
🏁 Script executed:
# Check if there are other check_run_handler.set_check_* calls without exception handling
rg -n "set_check_(in_progress|failure|success)" webhook_server/libs/handlers/runner_handler.pyRepository: myk-org/github-webhook-server
Length of output: 1700
CRITICAL: run_check() leaks user-defined command secrets via debug logging and can leave checks stuck in_progress if exceptions occur.
User-defined commands in repo YAML may contain tokens or credentials. Line 228 logs the full command in plaintext before run_command() applies masking. Additionally, no exception handling wraps the entire method—if set_check_in_progress() or any other operation raises unexpectedly, the check remains in_progress indefinitely, blocking re-runs.
The fix must:
- Remove the unmasked command debug log (line 228)
- Wrap the entire method in try/except to ensure check completion on unexpected failures
- Re-raise
asyncio.CancelledErrorper framework guidelines - Catch general exceptions and mark the check as failed with the error text before re-raising
This pattern is already established in pull_request_handler.py (lines 1060–1066) and issue_comment_handler.py (lines 135–140).
Proposed fix
async def run_check(self, pull_request: PullRequest, check_config: CheckConfig) -> None:
"""Unified check execution method for both built-in and custom checks.
This method handles the common lifecycle for all command-based checks:
1. Set check to in_progress
2. Checkout worktree
3. Execute command
4. Report success or failure
Args:
pull_request: The pull request to run the check on.
check_config: Configuration for the check (name, command, title, use_cwd).
"""
+ try:
if await self.check_run_handler.is_check_run_in_progress(check_run=check_config.name):
self.logger.debug(f"{self.log_prefix} Check run is in progress, re-running {check_config.name}.")
self.logger.info(f"{self.log_prefix} Starting check: {check_config.name}")
await self.check_run_handler.set_check_in_progress(name=check_config.name)
async with self._checkout_worktree(pull_request=pull_request) as (success, worktree_path, out, err):
output: CheckRunOutput = {
"title": check_config.title,
"summary": "",
"text": None,
}
if not success:
self.logger.error(f"{self.log_prefix} Repository preparation failed for {check_config.name}")
output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err)
return await self.check_run_handler.set_check_failure(name=check_config.name, output=output)
# Build command with worktree path substitution
# Use replace() instead of format() to avoid KeyError on other braces in user commands
cmd = check_config.command.replace("{worktree_path}", worktree_path)
- self.logger.debug(f"{self.log_prefix} {check_config.name} command to run: {cmd}")
+ # Avoid leaking user-configured secrets in logs for custom checks
+ 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
rc, out, err = await run_command(
command=cmd,
log_prefix=self.log_prefix,
mask_sensitive=self.github_webhook.mask_sensitive,
cwd=cwd,
)
output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out)
if rc:
self.logger.info(f"{self.log_prefix} Check {check_config.name} completed successfully")
return await self.check_run_handler.set_check_success(name=check_config.name, output=output)
else:
self.logger.info(f"{self.log_prefix} Check {check_config.name} failed")
return await self.check_run_handler.set_check_failure(name=check_config.name, output=output)
+ except asyncio.CancelledError:
+ raise
+ except Exception as ex:
+ self.logger.exception(f"{self.log_prefix} Unexpected failure running check {check_config.name}: {ex}")
+ output: CheckRunOutput = {"title": check_config.title, "summary": "", "text": str(ex)}
+ await self.check_run_handler.set_check_failure(name=check_config.name, output=output)
+ raise🧰 Tools
🪛 Ruff (0.14.10)
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)
240-240: Logging statement uses f-string
(G004)
243-243: Logging statement uses f-string
(G004)
| async def run_retests(self, supported_retests: list[str], pull_request: PullRequest) -> None: | ||
| """Run the specified retests for a pull request. | ||
| Args: | ||
| supported_retests: List of test names to run (e.g., ['tox', 'pre-commit']) | ||
| pull_request: The PullRequest object to run tests for | ||
| """ | ||
| if not supported_retests: | ||
| self.logger.debug(f"{self.log_prefix} No retests to run") | ||
| return | ||
|
|
||
| # Map check names to runner functions | ||
| _retests_to_func_map: dict[str, Callable[..., Coroutine[Any, Any, None]]] = { | ||
| TOX_STR: self.run_tox, | ||
| PRE_COMMIT_STR: self.run_pre_commit, | ||
| BUILD_CONTAINER_STR: self.run_build_container, | ||
| PYTHON_MODULE_INSTALL_STR: self.run_install_python_module, | ||
| CONVENTIONAL_TITLE_STR: self.run_conventional_title_check, | ||
| } | ||
|
|
||
| # Add custom check runs to the retest map | ||
| # Note: custom checks are validated in GithubWebhook._validate_custom_check_runs() | ||
| # so name is guaranteed to exist | ||
| for custom_check in self.github_webhook.custom_check_runs: | ||
| check_key = custom_check["name"] | ||
| _retests_to_func_map[check_key] = partial(self.run_custom_check, check_config=custom_check) | ||
|
|
||
| tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] | ||
| for _test in supported_retests: | ||
| runner = _retests_to_func_map.get(_test) | ||
| if runner is None: | ||
| self.logger.error(f"{self.log_prefix} Unknown retest '{_test}' requested, skipping") | ||
| continue | ||
| self.logger.debug(f"{self.log_prefix} running retest {_test}") | ||
| task = asyncio.create_task(runner(pull_request=pull_request)) | ||
| tasks.append(task) | ||
|
|
||
| results = await asyncio.gather(*tasks, return_exceptions=True) | ||
| for result in results: | ||
| if isinstance(result, Exception): | ||
| self.logger.error(f"{self.log_prefix} Async task failed: {result}") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.11 asyncio.CancelledError inheritance BaseException Exception
💡 Result:
In Python 3.11, asyncio.CancelledError is a subclass of BaseException (not Exception). So except Exception: will not catch it — you must catch asyncio.CancelledError (or BaseException) explicitly. [1][2]
Sources:
- Python 3.11 asyncio exceptions docs. [1]
- Explanation / behavior note (change since 3.8). [2]
🏁 Script executed:
# Check project Python version requirements and the handler file
fd pyproject.toml | head -5 | xargs cat
# Verify the file exists and check surrounding context
fd runner_handler.py | head -5Repository: myk-org/github-webhook-server
Length of output: 2752
🏁 Script executed:
# Verify the handler file content around the specified lines
cat -n webhook_server/libs/handlers/runner_handler.py | sed -n '595,645p'
# Check if there are other asyncio.gather patterns in this file
rg -n "asyncio.gather" webhook_server/libs/handlers/runner_handler.py -A 3Repository: myk-org/github-webhook-server
Length of output: 2812
HIGH: run_retests() must re-raise asyncio.CancelledError instead of silently swallowing it.
In Python 3.13, asyncio.CancelledError inherits from BaseException, not Exception. The current code uses isinstance(result, Exception), which does not catch CancelledError. This means cancellation signals from child tasks are silently ignored, breaking shutdown semantics and potentially leaving partial workflows running. Per project requirements, asyncio.CancelledError must always be re-raised.
Required 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}")🧰 Tools
🪛 Ruff (0.14.10)
607-607: Logging statement uses f-string
(G004)
630-630: Logging statement uses f-string
(G004)
632-632: Logging statement uses f-string
(G004)
639-639: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In @webhook_server/libs/handlers/runner_handler.py around lines 599 - 639, In
run_retests, the post-gather error handling must re-raise asyncio.CancelledError
instead of treating it as a regular Exception; change the results loop that
currently does "for result in results: if isinstance(result, Exception): ..." to
explicitly detect and re-raise asyncio.CancelledError (and any other
BaseException that isn’t an Exception) while continuing to log normal Exception
instances via self.logger.error; reference the results variable and the
run_retests method and ensure you check isinstance(result,
asyncio.CancelledError) first, then if isinstance(result, Exception) log it,
else re-raise the result.
| @pytest.fixture | ||
| def mock_github_webhook(self, tmp_path: Any) -> Mock: | ||
| """Create a mock GithubWebhook instance.""" | ||
| mock_webhook = Mock() | ||
| mock_webhook.hook_data = {} | ||
| mock_webhook.logger = Mock() | ||
| mock_webhook.log_prefix = "[TEST]" | ||
| mock_webhook.repository = Mock() | ||
| mock_webhook.clone_repo_dir = str(tmp_path / "test-repo") | ||
| mock_webhook.mask_sensitive = True | ||
| return mock_webhook | ||
|
|
There was a problem hiding this comment.
HIGH: Replace tmp_path: Any with Path to satisfy strict typing (ANN401).
In pytest, tmp_path is a pathlib.Path. Keeping it typed prevents Any from leaking through tests.
Proposed fix
import asyncio
+from pathlib import Path
from typing import Any
from unittest.mock import AsyncMock, Mock, patch
...
class TestRunnerHandlerCustomCheck:
...
@pytest.fixture
- def mock_github_webhook(self, tmp_path: Any) -> Mock:
+ def mock_github_webhook(self, tmp_path: Path) -> Mock:
...
class TestCustomCheckRunsIntegration:
...
@pytest.fixture
- def mock_github_webhook(self, tmp_path: Any) -> Mock:
+ def mock_github_webhook(self, tmp_path: Path) -> Mock:
...
class TestCustomCheckRunsEdgeCases:
...
@pytest.fixture
- def mock_github_webhook(self, tmp_path: Any) -> Mock:
+ def mock_github_webhook(self, tmp_path: Path) -> Mock:Also applies to: 529-552, 1019-1032
🧰 Tools
🪛 Ruff (0.14.10)
387-387: Dynamically typed expressions (typing.Any) are disallowed in tmp_path
(ANN401)
🤖 Prompt for AI Agents
In @webhook_server/tests/test_custom_check_runs.py around lines 386 - 397, The
fixture mock_github_webhook uses tmp_path annotated as Any which fails ANN401;
change the parameter type to pathlib.Path (i.e., tmp_path: Path) and add the
corresponding from pathlib import Path import at the top of the test module;
update the same pattern in the other fixtures mentioned (the blocks around lines
529-552 and 1019-1032) so all tmp_path parameters are typed as Path to avoid
leaking Any.
| # Create async context manager mock | ||
| mock_checkout_cm = AsyncMock() | ||
| mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, "/tmp/worktree", "", "")) | ||
| mock_checkout_cm.__aexit__ = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch.object(runner_handler, "_checkout_worktree", return_value=mock_checkout_cm), | ||
| patch( | ||
| "webhook_server.libs.handlers.runner_handler.run_command", | ||
| new=AsyncMock(return_value=(True, "output", "")), | ||
| ) as mock_run, | ||
| ): | ||
| await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) | ||
|
|
||
| # Verify check run status updates | ||
| runner_handler.check_run_handler.set_check_in_progress.assert_called_once_with(name="lint") | ||
| runner_handler.check_run_handler.set_check_success.assert_called_once() | ||
|
|
||
| # Verify command was executed | ||
| mock_run.assert_called_once() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_run_custom_check_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: | ||
| """Test failed execution of custom check.""" | ||
| check_config = { | ||
| "name": "security-scan", | ||
| "command": "uv tool run --from bandit bandit -r .", | ||
| } | ||
|
|
||
| # Create async context manager mock | ||
| mock_checkout_cm = AsyncMock() | ||
| mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, "/tmp/worktree", "", "")) | ||
| mock_checkout_cm.__aexit__ = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch.object(runner_handler, "_checkout_worktree", return_value=mock_checkout_cm), | ||
| patch( | ||
| "webhook_server.libs.handlers.runner_handler.run_command", | ||
| new=AsyncMock(return_value=(False, "output", "error message")), | ||
| ), | ||
| ): | ||
| await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) | ||
|
|
||
| # Verify failure status was set | ||
| runner_handler.check_run_handler.set_check_failure.assert_called_once() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_run_custom_check_checkout_failure( | ||
| self, runner_handler: RunnerHandler, mock_pull_request: Mock | ||
| ) -> None: | ||
| """Test custom check when repository checkout fails.""" | ||
| check_config = { | ||
| "name": "lint", | ||
| "command": "uv tool run --from pytest pytest", | ||
| } | ||
|
|
||
| # Create async context manager mock with failed checkout | ||
| mock_checkout_cm = AsyncMock() | ||
| mock_checkout_cm.__aenter__ = AsyncMock(return_value=(False, "", "checkout output", "checkout error")) | ||
| mock_checkout_cm.__aexit__ = AsyncMock(return_value=None) | ||
|
|
||
| with patch.object(runner_handler, "_checkout_worktree", return_value=mock_checkout_cm): | ||
| await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) | ||
|
|
||
| # Verify failure status was set due to checkout failure | ||
| runner_handler.check_run_handler.set_check_failure.assert_called_once() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_run_custom_check_command_execution_in_worktree( | ||
| self, runner_handler: RunnerHandler, mock_pull_request: Mock | ||
| ) -> None: | ||
| """Test that custom check command is executed in worktree directory.""" | ||
| check_config = { | ||
| "name": "build", | ||
| "command": "uv tool run --from build python -m build", | ||
| } | ||
|
|
||
| # Create async context manager mock | ||
| mock_checkout_cm = AsyncMock() | ||
| mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, "/tmp/test-worktree", "", "")) | ||
| mock_checkout_cm.__aexit__ = AsyncMock(return_value=None) | ||
|
|
||
| with ( | ||
| patch.object(runner_handler, "_checkout_worktree", return_value=mock_checkout_cm), | ||
| patch( | ||
| "webhook_server.libs.handlers.runner_handler.run_command", | ||
| new=AsyncMock(return_value=(True, "output", "")), | ||
| ) as mock_run, | ||
| ): | ||
| await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) | ||
|
|
||
| # Verify command is executed with cwd parameter set to worktree | ||
| mock_run.assert_called_once() | ||
| call_args = mock_run.call_args.kwargs | ||
| assert call_args["command"] == "uv tool run --from build python -m build" | ||
| assert call_args["cwd"] == "/tmp/test-worktree" | ||
|
|
There was a problem hiding this comment.
LOW: Avoid hardcoded /tmp/... paths in tests (S108) — prefer tmp_path.
Even though these are tests, using tmp_path improves portability and keeps linters quiet.
Proposed pattern
- mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, "/tmp/worktree", "", ""))
+ worktree = tmp_path / "worktree"
+ mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(worktree), "", ""))Apply similarly for "/tmp/test-worktree".
Also applies to: 572-593, 672-689, 1057-1125
🧰 Tools
🪛 Ruff (0.14.10)
429-429: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
458-458: Probable insecure usage of temporary file or directory: "/tmp/worktree"
(S108)
506-506: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
522-522: Probable insecure usage of temporary file or directory: "/tmp/test-worktree"
(S108)
The feature allows users to define custom checks in their YAML config that:
Summary by CodeRabbit
New Features
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.