diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3588e2ab6..4ff545454 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -55,7 +55,8 @@ repos: hooks: - id: mypy exclude: (tests/) - additional_dependencies: [types-requests, types-PyYAML, types-colorama, types-aiofiles] + additional_dependencies: + [types-requests, types-PyYAML, types-colorama, types-aiofiles] - repo: https://github.com/pre-commit/mirrors-eslint rev: v10.0.0-rc.0 diff --git a/README.md b/README.md index c5653a4de..4de373dd6 100644 --- a/README.md +++ b/README.md @@ -409,6 +409,85 @@ Invalid color names automatically fall back to `lightgray`. Configuration changes take effect immediately without server restart. The webhook server re-reads configuration for each incoming webhook event. +### Configurable Labels + +The webhook server supports enabling/disabling specific label categories and customizing label colors. This allows repository administrators to control which automation labels are applied to pull requests. + +#### Configuration Options + +```yaml +# Global configuration (applies to all repositories) +labels: + enabled-labels: + - verified + - hold + - wip + - needs-rebase + - has-conflicts + - can-be-merged + - size + - branch + - cherry-pick + - automerge + colors: + hold: red + verified: green + wip: orange + +# Repository-specific configuration (overrides global) +repositories: + my-project: + name: my-org/my-project + labels: + enabled-labels: + - verified + - wip + - size + colors: + verified: lightgreen +``` + +#### Available Label Categories + +| Category | Labels Applied | Description | +|----------|---------------|-------------| +| `verified` | `verified` | Manual verification status | +| `hold` | `hold` | Block PR merging | +| `wip` | `wip` | Work in progress status | +| `needs-rebase` | `needs-rebase` | PR needs rebasing | +| `has-conflicts` | `has-conflicts` | Merge conflicts detected | +| `can-be-merged` | `can-be-merged` | PR meets all merge requirements | +| `size` | `size/XS`, `size/S`, etc. | PR size labels | +| `branch` | `branch/` | Target branch labels | +| `cherry-pick` | `cherry-pick/` | Cherry-pick tracking | +| `automerge` | `automerge` | Auto-merge enabled | + +#### Configuration Rules + +- **enabled-labels**: Optional array of label categories to enable + - If omitted, ALL label categories are enabled (default behavior) + - If empty array `[]`, all configurable labels are disabled +- **colors**: Optional object mapping label names to CSS3 color names + - Supports any valid CSS3 color name (e.g., `red`, `lightblue`, `darkgreen`) + - Invalid color names fall back to default colors +- **reviewed-by labels**: Always enabled (`approved-*`, `lgtm-*`, `changes-requested-*`, `commented-*`) + - These are the source of truth for the approval system and cannot be disabled +- **Hierarchy**: Repository-level configuration overrides global configuration +- **Real-time Updates**: Changes take effect immediately without server restart + +#### Example: Minimal Labels Configuration + +```yaml +# Only enable essential labels +labels: + enabled-labels: + - verified + - can-be-merged + - size +``` + +This configuration disables `hold`, `wip`, `needs-rebase`, `has-conflicts`, `branch`, `cherry-pick`, and `automerge` labels. + ### Repository-Level Overrides Create `.github-webhook-server.yaml` in your repository root to override or extend the global configuration for that specific repository. This file supports all repository-level configuration options. @@ -428,6 +507,16 @@ set-auto-merge-prs: pre-commit: true conventional-title: "feat,fix,docs" +# Label configuration +labels: + enabled-labels: + - verified + - hold + - wip + colors: + hold: crimson + verified: limegreen + # Custom PR size labels for this repository pr-size-thresholds: Quick: diff --git a/examples/config.yaml b/examples/config.yaml index 86f6c39b7..7303753f0 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -33,6 +33,40 @@ auto-verify-cherry-picked-prs: true # Default: true - automatically verify cher create-issue-for-new-pr: true # Global default: create tracking issues for new PRs +# Labels configuration - control which labels are enabled and their colors +# If not set, all labels are enabled with default colors +labels: + # Optional: List of label categories to enable + # If not set, all labels are enabled. If set, only listed categories are enabled. + # Note: reviewed-by labels (approved-*, lgtm-*, etc.) are always enabled and cannot be disabled + enabled-labels: + - verified + - hold + - wip + - needs-rebase + - has-conflicts + - can-be-merged + - size + - branch + - cherry-pick + - automerge + # Optional: Custom colors for labels (CSS3 color names) + colors: + hold: red + verified: green + wip: orange + needs-rebase: darkred + has-conflicts: red + can-be-merged: limegreen + automerge: green + # Dynamic label prefixes + approved-: green + lgtm-: yellowgreen + changes-requested-: orange + commented-: gold + cherry-pick-: coral + branch-: royalblue + # Global PR size label configuration (optional) # Define custom categories based on total lines changed (additions + deletions) # threshold: positive integer or 'inf' for unbounded largest category @@ -152,6 +186,15 @@ repositories: minimum-lgtm: 0 # The minimum PR lgtm required before approve the PR create-issue-for-new-pr: true # Override global setting: create tracking issues for new PRs (default: true) + # Repository-specific labels configuration (overrides global) + labels: + enabled-labels: + - verified + - hold + - size + colors: + hold: purple + # Repository-specific PR size labels (overrides global configuration) pr-size-thresholds: Express: diff --git a/pyproject.toml b/pyproject.toml index 4b7185fcf..4f3631c43 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ exclude = [".git", ".venv", ".mypy_cache", ".tox", "__pycache__"] [tool.mypy] check_untyped_defs = true -disallow_any_generics = false +disallow_any_generics = true disallow_incomplete_defs = true disallow_untyped_defs = true no_implicit_optional = true diff --git a/scripts/generate_changelog.py b/scripts/generate_changelog.py index b3a0151ac..9228d015e 100644 --- a/scripts/generate_changelog.py +++ b/scripts/generate_changelog.py @@ -3,9 +3,10 @@ import subprocess import sys from collections import OrderedDict +from typing import Any -def json_line(line: str) -> dict: +def json_line(line: str) -> dict[str, Any]: """ Format str line to str that can be parsed with json. @@ -59,28 +60,27 @@ def execute_git_log(from_tag: str, to_tag: str) -> str: sys.exit(1) -def parse_commit_line(line: str) -> dict: +def parse_commit_line(line: str) -> dict[str, Any]: """Parses a single JSON formatted git log line.""" try: return json_line(line=line) - except json.decoder.JSONDecodeError as ex: + except json.JSONDecodeError as ex: print(f"Error parsing JSON: {line} - {ex}") return {} -def categorize_commit(commit: dict, title_to_type_map: dict, default_category: str = "Other Changes:") -> str: +def categorize_commit( + commit: dict[str, Any], title_to_type_map: dict[str, str], default_category: str = "Other Changes:" +) -> str: """Categorizes a commit based on its title prefix.""" if not commit or "title" not in commit: return default_category title = commit["title"] - try: - prefix = title.split(":", 1)[0].lower() # Extract the prefix before the first colon - return title_to_type_map.get(prefix, default_category) - except IndexError: - return default_category + prefix = title.split(":", 1)[0].lower() # Extract the prefix before the first colon + return title_to_type_map.get(prefix, default_category) -def format_changelog_entry(change: dict, section: str) -> str: +def format_changelog_entry(change: dict[str, Any], section: str) -> str: """Formats a single changelog entry.""" title = change["title"].split(":", 1)[1] if section != "Other Changes:" else change["title"] return f"- {title} ({change['commit']}) by {change['author']} on {change['date']}\n" diff --git a/webhook_server/app.py b/webhook_server/app.py index 5a3405608..4ea6a777f 100644 --- a/webhook_server/app.py +++ b/webhook_server/app.py @@ -7,6 +7,7 @@ from collections.abc import AsyncGenerator from contextlib import asynccontextmanager from datetime import UTC, datetime +from ipaddress import IPv4Network, IPv6Network from typing import Any import httpx @@ -58,11 +59,11 @@ MCP_SERVER_ENABLED: bool = os.environ.get("ENABLE_MCP_SERVER") == "true" # Global variables -ALLOWED_IPS: tuple[ipaddress._BaseNetwork, ...] = () +ALLOWED_IPS: tuple[IPv4Network | IPv6Network, ...] = () LOGGER = get_logger_with_params() _lifespan_http_client: httpx.AsyncClient | None = None -_background_tasks: set[asyncio.Task] = set() +_background_tasks: set[asyncio.Task[Any]] = set() # MCP Globals http_transport: Any | None = None @@ -160,7 +161,7 @@ async def lifespan(_app: FastAPI) -> AsyncGenerator[None]: LOGGER.debug(f"verify_github_ips: {verify_github_ips}, verify_cloudflare_ips: {verify_cloudflare_ips}") global ALLOWED_IPS - networks: set[ipaddress._BaseNetwork] = set() + networks: set[IPv4Network | IPv6Network] = set() if verify_cloudflare_ips: try: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index c63156aa2..f4d0718f2 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -83,6 +83,39 @@ properties: type: boolean description: Create a tracking issue for new pull requests (global default) default: true + labels: + type: object + description: Configure which labels are enabled and their colors + properties: + enabled-labels: + type: array + description: | + List of label categories to enable. If not set, all labels are enabled. + Categories: verified, hold, wip, needs-rebase, has-conflicts, can-be-merged, + size, branch, cherry-pick, automerge. + Note: reviewed-by labels (approved-*, lgtm-*, changes-requested-*, commented-*) + are always enabled and cannot be disabled. + items: + type: string + enum: + - verified + - hold + - wip + - needs-rebase + - has-conflicts + - can-be-merged + - size + - branch + - cherry-pick + - automerge + colors: + type: object + description: | + Custom colors for labels (CSS3 color names like 'red', 'green', 'blue'). + Use label name as key, color as value. For dynamic labels use prefix (e.g., 'approved-', 'branch-'). + additionalProperties: + type: string + additionalProperties: false pr-size-thresholds: type: object @@ -299,6 +332,39 @@ properties: type: boolean description: Create a tracking issue for new pull requests default: true + labels: + type: object + description: Configure which labels are enabled and their colors + properties: + enabled-labels: + type: array + description: | + List of label categories to enable. If not set, all labels are enabled. + Categories: verified, hold, wip, needs-rebase, has-conflicts, can-be-merged, + size, branch, cherry-pick, automerge. + Note: reviewed-by labels (approved-*, lgtm-*, changes-requested-*, commented-*) + are always enabled and cannot be disabled. + items: + type: string + enum: + - verified + - hold + - wip + - needs-rebase + - has-conflicts + - can-be-merged + - size + - branch + - cherry-pick + - automerge + colors: + type: object + description: | + Custom colors for labels (CSS3 color names like 'red', 'green', 'blue'). + Use label name as key, color as value. For dynamic labels use prefix (e.g., 'approved-', 'branch-'). + additionalProperties: + type: string + additionalProperties: false pr-size-thresholds: type: object description: Custom PR size thresholds with label names and colors (repository-specific override) diff --git a/webhook_server/libs/config.py b/webhook_server/libs/config.py index 4686a0541..6fef1c6d5 100644 --- a/webhook_server/libs/config.py +++ b/webhook_server/libs/config.py @@ -7,6 +7,8 @@ from github.GithubException import UnknownObjectException from simple_logger.logger import get_logger +from webhook_server.utils.constants import CONFIGURABLE_LABEL_CATEGORIES + class Config: def __init__( @@ -20,6 +22,7 @@ def __init__( self.repository = repository self.exists() self.repositories_exists() + self.validate_labels_config() def exists(self) -> None: if not os.path.isfile(self.config_path): @@ -29,6 +32,36 @@ def repositories_exists(self) -> None: if not self.root_data.get("repositories"): raise ValueError(f"Config {self.config_path} does not have `repositories`") + def validate_labels_config(self) -> None: + """Validate enabled-labels configuration against allowed categories. + + Raises: + ValueError: If any label category in enabled-labels is not valid. + """ + data = self.root_data + + # Check global labels config + if "labels" in data and "enabled-labels" in data["labels"]: + enabled_labels = set(data["labels"]["enabled-labels"]) + invalid = enabled_labels - CONFIGURABLE_LABEL_CATEGORIES + if invalid: + raise ValueError( + f"Invalid label categories in enabled-labels: {sorted(invalid)}. " + f"Valid categories: {sorted(CONFIGURABLE_LABEL_CATEGORIES)}" + ) + + # Check repository-level labels config + for repo_name, repo_config in data.get("repositories", {}).items(): + if isinstance(repo_config, dict) and "labels" in repo_config: + if "enabled-labels" in repo_config["labels"]: + enabled_labels = set(repo_config["labels"]["enabled-labels"]) + invalid = enabled_labels - CONFIGURABLE_LABEL_CATEGORIES + if invalid: + raise ValueError( + f"Invalid label categories in enabled-labels for repository '{repo_name}': " + f"{sorted(invalid)}. Valid categories: {sorted(CONFIGURABLE_LABEL_CATEGORIES)}" + ) + @property def root_data(self) -> dict[str, Any]: try: diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 15c7d30fe..1faefe3b4 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -9,6 +9,7 @@ import tempfile import threading import traceback +from asyncio import Task from typing import Any import github @@ -29,6 +30,7 @@ from webhook_server.utils.constants import ( BUILD_CONTAINER_STR, CAN_BE_MERGED_STR, + CONFIGURABLE_LABEL_CATEGORIES, CONVENTIONAL_TITLE_STR, OTHER_MAIN_BRANCH, PRE_COMMIT_STR, @@ -82,7 +84,7 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. self.hook_data = hook_data self.repository_name: str = hook_data["repository"]["name"] self.repository_full_name: str = hook_data["repository"]["full_name"] - self._bg_tasks: set[asyncio.Task] = set() + self._bg_tasks: set[Task[Any]] = set() self.parent_committer: str = "" self.x_github_delivery: str = headers.get("X-GitHub-Delivery", "") self.github_event: str = headers["X-GitHub-Event"] @@ -289,7 +291,8 @@ async def _clone_repository( try: github_token = self.token - clone_url_with_token = self.repository.clone_url.replace("https://", f"https://{github_token}@") + clone_url = await asyncio.to_thread(lambda: self.repository.clone_url) + clone_url_with_token = clone_url.replace("https://", f"https://{github_token}@") rc, _, err = await run_command( command=f"git clone {clone_url_with_token} {self.clone_repo_dir}", @@ -308,8 +311,9 @@ def redact_output(value: str) -> str: # Configure git user git_cmd = f"git -C {self.clone_repo_dir}" + owner_login = await asyncio.to_thread(lambda: self.repository.owner.login) rc, _, _ = await run_command( - command=f"{git_cmd} config user.name '{self.repository.owner.login}'", + command=f"{git_cmd} config user.name '{owner_login}'", log_prefix=self.log_prefix, mask_sensitive=self.mask_sensitive, ) @@ -317,42 +321,58 @@ def redact_output(value: str) -> str: self.logger.warning(f"{self.log_prefix} Failed to configure git user.name") rc, _, _ = await run_command( - command=f"{git_cmd} config user.email '{self.repository.owner.login}@users.noreply.github.com'", + command=f"{git_cmd} config user.email '{owner_login}@users.noreply.github.com'", log_prefix=self.log_prefix, mask_sensitive=self.mask_sensitive, ) if not rc: self.logger.warning(f"{self.log_prefix} Failed to configure git user.email") - # Configure PR fetch to enable origin/pr/* checkouts - rc, _, _ = await run_command( - command=( - f"{git_cmd} config --local --add remote.origin.fetch +refs/pull/*/head:refs/remotes/origin/pr/*" - ), - log_prefix=self.log_prefix, - mask_sensitive=self.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Failed to configure PR fetch refs") + # Fetch only what's needed instead of all refs + if pull_request: + # Fetch the base branch first (needed for checkout) + base_ref = await asyncio.to_thread(lambda: pull_request.base.ref) + rc, _, err = await run_command( + command=f"{git_cmd} fetch origin {base_ref}", + log_prefix=self.log_prefix, + mask_sensitive=self.mask_sensitive, + ) + if not rc: + redacted_err = redact_output(err) + self.logger.error(f"{self.log_prefix} Failed to fetch base branch {base_ref}: {redacted_err}") + raise RuntimeError(f"Failed to fetch base branch {base_ref}: {redacted_err}") - # Fetch all refs including PRs - rc, _, _ = await run_command( - command=f"{git_cmd} remote update", - log_prefix=self.log_prefix, - mask_sensitive=self.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Failed to fetch remote refs") + # Fetch only this specific PR's ref + pr_number = await asyncio.to_thread(lambda: pull_request.number) + rc, _, err = await run_command( + command=f"{git_cmd} fetch origin +refs/pull/{pr_number}/head:refs/remotes/origin/pr/{pr_number}", + log_prefix=self.log_prefix, + mask_sensitive=self.mask_sensitive, + ) + if not rc: + redacted_err = redact_output(err) + self.logger.error(f"{self.log_prefix} Failed to fetch PR {pr_number} ref: {redacted_err}") + raise RuntimeError(f"Failed to fetch PR {pr_number} ref: {redacted_err}") + else: + # For push events (tags only - branch pushes skip cloning) + # checkout_ref guaranteed to be non-None by validation at function start + tag_name = checkout_ref.replace("refs/tags/", "") # type: ignore[union-attr] + fetch_refspec = f"refs/tags/{tag_name}:refs/tags/{tag_name}" + rc, _, _ = await run_command( + command=f"{git_cmd} fetch origin {fetch_refspec}", + log_prefix=self.log_prefix, + mask_sensitive=self.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Failed to fetch tag {checkout_ref}") # Determine checkout target if pull_request: checkout_target = await asyncio.to_thread(lambda: pull_request.base.ref) else: - # For push events: "refs/tags/v11.0.104" → "v11.0.104" - # "refs/heads/main" → "main" + # For push events (tags only - branch pushes skip cloning) # checkout_ref guaranteed to be non-None by validation at function start - assert checkout_ref is not None # mypy type narrowing - checkout_target = checkout_ref.replace("refs/tags/", "").replace("refs/heads/", "") + checkout_target = checkout_ref.replace("refs/tags/", "") # type: ignore[union-attr] # Checkout target branch/tag rc, _, err = await run_command( @@ -414,14 +434,24 @@ async def process(self) -> Any: await self._update_context_metrics() return None - # Clone repository for push operations (PyPI uploads, container builds) - await self._clone_repository(checkout_ref=self.hook_data["ref"]) + ref = self.hook_data["ref"] + + # Only clone for tag pushes - branch pushes don't require cloning + # because PushHandler only processes tags (PyPI upload, container build) + if ref.startswith("refs/tags/"): + await self._clone_repository(checkout_ref=ref) + await PushHandler(github_webhook=self).process_push_webhook_data() + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} Webhook processing completed successfully: push - {token_metrics}", + ) + else: + self.logger.debug(f"{self.log_prefix} Skipping clone for branch push: {ref}") + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} Webhook processing completed: branch push (skipped) - {token_metrics}", + ) - await PushHandler(github_webhook=self).process_push_webhook_data() - token_metrics = await self._get_token_metrics() - self.logger.info( - f"{self.log_prefix} Webhook processing completed successfully: push - {token_metrics}", - ) await self._update_context_metrics() return None @@ -586,17 +616,18 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: self.logger.debug(f"Read config for repository {self.repository_name}") self.github_app_id: str = self.config.get_value(value="github-app-id", extra_dict=repository_config) - self.pypi: dict[str, str] = self.config.get_value(value="pypi", extra_dict=repository_config) + _pypi = self.config.get_value(value="pypi", extra_dict=repository_config) + self.pypi: dict[str, str] = _pypi if isinstance(_pypi, dict) else {} self.verified_job: bool = self.config.get_value( value="verified-job", return_on_none=True, extra_dict=repository_config ) - self.tox: dict[str, str] = self.config.get_value(value="tox", extra_dict=repository_config) + _tox = self.config.get_value(value="tox", extra_dict=repository_config) + self.tox: dict[str, str] = _tox if isinstance(_tox, dict) else {} self.tox_python_version: str = self.config.get_value(value="tox-python-version", extra_dict=repository_config) self.slack_webhook_url: str = self.config.get_value(value="slack-webhook-url", extra_dict=repository_config) - self.build_and_push_container: dict[str, Any] = self.config.get_value( - value="container", return_on_none={}, extra_dict=repository_config - ) + _container = self.config.get_value(value="container", return_on_none={}, extra_dict=repository_config) + self.build_and_push_container: dict[str, Any] = _container if isinstance(_container, dict) else {} if self.build_and_push_container: self.container_repository_username: str = self.build_and_push_container["username"] self.container_repository_password: str = self.build_and_push_container["password"] @@ -622,19 +653,22 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: value="pre-commit", return_on_none=False, extra_dict=repository_config ) - self.auto_verified_and_merged_users: list[str] = self.config.get_value( + _auto_users = self.config.get_value( value="auto-verified-and-merged-users", return_on_none=[], extra_dict=repository_config ) + self.auto_verified_and_merged_users: list[str] = _auto_users if isinstance(_auto_users, list) else [] self.auto_verify_cherry_picked_prs: bool = self.config.get_value( value="auto-verify-cherry-picked-prs", return_on_none=True, extra_dict=repository_config ) - self.can_be_merged_required_labels = self.config.get_value( + _required_labels = self.config.get_value( value="can-be-merged-required-labels", return_on_none=[], extra_dict=repository_config ) + self.can_be_merged_required_labels: list[str] = _required_labels if isinstance(_required_labels, list) else [] self.conventional_title: str = self.config.get_value(value="conventional-title", extra_dict=repository_config) - self.set_auto_merge_prs: list[str] = self.config.get_value( + _auto_merge_prs = self.config.get_value( value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config ) + self.set_auto_merge_prs: list[str] = _auto_merge_prs if isinstance(_auto_merge_prs, list) else [] self.minimum_lgtm: int = self.config.get_value( value="minimum-lgtm", return_on_none=0, extra_dict=repository_config ) @@ -647,6 +681,63 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: value="create-issue-for-new-pr", return_on_none=global_create_issue_for_new_pr, extra_dict=repository_config ) + # Load labels configuration + _global_labels = self.config.get_value("labels", return_on_none={}) + global_labels_config: dict[str, Any] = _global_labels if isinstance(_global_labels, dict) else {} + _repo_labels = self.config.get_value("labels", return_on_none={}, extra_dict=repository_config) + repo_labels_config: dict[str, Any] = _repo_labels if isinstance(_repo_labels, dict) else {} + + # Merge global and repo labels config (repo overrides global) + merged_labels_config = {**global_labels_config, **repo_labels_config} + + # enabled-labels: if not set, all labels enabled (None means all enabled) + self.enabled_labels: set[str] | None = None + _enabled_labels = merged_labels_config.get("enabled-labels") + if _enabled_labels is not None: + if isinstance(_enabled_labels, list): + # Filter non-string entries to avoid TypeError from unhashable items (e.g., dicts from YAML mistakes) + enabled_set = {x for x in _enabled_labels if isinstance(x, str)} + dropped = [x for x in _enabled_labels if not isinstance(x, str)] + if dropped: + # Sanitize dropped items for safe logging (avoid large/untrusted YAML blobs) + max_repr_len = 50 + + def _sanitize_item(item: Any) -> str: + if isinstance(item, dict): + keys = list(item.keys())[:5] + return f"dict(keys={keys})" + elif isinstance(item, list): + return f"list(len={len(item)})" + else: + type_name = type(item).__name__ + item_repr = repr(item) + if len(item_repr) > max_repr_len: + item_repr = item_repr[:max_repr_len] + "..." + return f"{type_name}({item_repr})" + + sanitized_dropped = [_sanitize_item(x) for x in dropped] + log_prefix = getattr(self, "log_prefix", "") + self.logger.warning( + f"{log_prefix} Non-string entries in enabled-labels were ignored: {sanitized_dropped}" + ) + # Log warning for invalid categories + invalid = enabled_set - CONFIGURABLE_LABEL_CATEGORIES + if invalid: + log_prefix = getattr(self, "log_prefix", "") + self.logger.warning( + f"{log_prefix} Invalid label categories in enabled-labels config: {invalid}. " + f"Valid categories: {CONFIGURABLE_LABEL_CATEGORIES}" + ) + self.enabled_labels = enabled_set & CONFIGURABLE_LABEL_CATEGORIES # Only keep valid categories + + # colors: deep-merge global defaults with repo overrides + _global_colors = global_labels_config.get("colors", {}) + _repo_colors = repo_labels_config.get("colors", {}) + global_colors = _global_colors if isinstance(_global_colors, dict) else {} + repo_colors = _repo_colors if isinstance(_repo_colors, dict) else {} + merged_colors = {**global_colors, **repo_colors} + self.label_colors: dict[str, str] = {str(k): str(v) for k, v in merged_colors.items()} + self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) async def get_pull_request(self, number: int | None = None) -> PullRequest | None: diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index 50af9bbe0..a0dcecf4e 100644 --- a/webhook_server/libs/handlers/check_run_handler.py +++ b/webhook_server/libs/handlers/check_run_handler.py @@ -12,7 +12,7 @@ AUTOMERGE_LABEL_STR, BUILD_CONTAINER_STR, CAN_BE_MERGED_STR, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, FAILURE_STR, IN_PROGRESS_STR, @@ -201,17 +201,13 @@ 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_PREFIX, status=IN_PROGRESS_STR) + 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_PREFIX, conclusion=SUCCESS_STR, output=output - ) + 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_PREFIX, conclusion=FAILURE_STR, output=output - ) + return await self.set_check_run_status(check_run=CHERRY_PICKED_LABEL, conclusion=FAILURE_STR, output=output) async def set_check_run_status( self, @@ -286,8 +282,11 @@ def get_check_run_text(self, err: str, out: str) -> str: _hased_str = "*****" - if self.github_webhook.pypi and self.github_webhook.pypi.get("token"): - _output = _output.replace(self.github_webhook.pypi["token"], _hased_str) + pypi_config = self.github_webhook.pypi + if isinstance(pypi_config, dict): + pypi_token = pypi_config.get("token") + if pypi_token: + _output = _output.replace(pypi_token, _hased_str) if getattr(self.github_webhook, "container_repository_username", None): _output = _output.replace(self.github_webhook.container_repository_username, _hased_str) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 7c33240fd..a55f02a15 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -24,6 +24,7 @@ COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REGENERATE_WELCOME_STR, COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, CONVENTIONAL_TITLE_STR, @@ -158,6 +159,7 @@ async def user_commands( BUILD_AND_PUSH_CONTAINER_STR, COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ADD_ALLOWED_USER_STR, + COMMAND_REGENERATE_WELCOME_STR, ] command_and_args: list[str] = command.split(" ", 1) @@ -238,6 +240,14 @@ async def user_commands( return await self.pull_request_handler.process_command_reprocess(pull_request=pull_request) + elif _command == COMMAND_REGENERATE_WELCOME_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return + self.logger.info(f"{self.log_prefix} Regenerating welcome message") + await self.pull_request_handler.regenerate_welcome_message(pull_request=pull_request) + elif _command == BUILD_AND_PUSH_CONTAINER_STR: if self.github_webhook.build_and_push_container: await self.runner_handler.run_build_container( @@ -256,13 +266,24 @@ async def user_commands( elif _command == WIP_STR: wip_for_title: str = f"{WIP_STR.upper()}:" if remove: - await self.labels_handler._remove_label(pull_request=pull_request, label=WIP_STR) - pr_title = await asyncio.to_thread(lambda: pull_request.title) - await asyncio.to_thread(pull_request.edit, title=pr_title.replace(wip_for_title, "")) + label_changed = await self.labels_handler._remove_label(pull_request=pull_request, label=WIP_STR) + if label_changed: + pr_title = await asyncio.to_thread(lambda: pull_request.title) + # Case-insensitive check and removal of WIP prefix + pr_title_upper = pr_title.upper() + if pr_title_upper.startswith("WIP: "): + new_title = pr_title[5:] # Remove "WIP: " (5 chars) + await asyncio.to_thread(pull_request.edit, title=new_title) + elif pr_title_upper.startswith("WIP:"): + new_title = pr_title[4:] # Remove "WIP:" (4 chars) + await asyncio.to_thread(pull_request.edit, title=new_title) else: - await self.labels_handler._add_label(pull_request=pull_request, label=WIP_STR) - pr_title = await asyncio.to_thread(lambda: pull_request.title) - await asyncio.to_thread(pull_request.edit, title=f"{wip_for_title} {pr_title}") + label_changed = await self.labels_handler._add_label(pull_request=pull_request, label=WIP_STR) + if label_changed: + pr_title = await asyncio.to_thread(lambda: pull_request.title) + # Case-insensitive check: only prepend if prefix is not already there (idempotent) + if not pr_title.upper().startswith("WIP:"): + await asyncio.to_thread(pull_request.edit, title=f"{wip_for_title} {pr_title}") elif _command == HOLD_LABEL_STR: if reviewed_user not in self.owners_file_handler.all_pull_request_approvers: @@ -279,15 +300,19 @@ async def user_commands( else: await self.labels_handler._add_label(pull_request=pull_request, label=HOLD_LABEL_STR) - await self.pull_request_handler.check_if_can_be_merged(pull_request=pull_request) - elif _command == VERIFIED_LABEL_STR: if remove: - await self.labels_handler._remove_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) - await self.check_run_handler.set_verify_check_queued() + 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() else: - await self.labels_handler._add_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) - await self.check_run_handler.set_verify_check_success() + 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() elif _command != AUTOMERGE_LABEL_STR: await self.labels_handler.label_by_user_comment( @@ -405,7 +430,7 @@ 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] = { + _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, diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index f1e01693b..8e520cb74 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -12,11 +12,15 @@ ADD_STR, APPROVE_STR, APPROVED_BY_LABEL_PREFIX, + BRANCH_LABEL_PREFIX, CHANGED_REQUESTED_BY_LABEL_PREFIX, + CHERRY_PICK_LABEL_PREFIX, + CHERRY_PICKED_LABEL, COMMENTED_BY_LABEL_PREFIX, + DEFAULT_LABEL_COLORS, DELETE_STR, - DYNAMIC_LABELS_DICT, HOLD_LABEL_STR, + LABEL_CATEGORY_MAP, LGTM_BY_LABEL_PREFIX, LGTM_STR, SIZE_LABEL_PREFIX, @@ -27,6 +31,16 @@ if TYPE_CHECKING: from webhook_server.libs.github_api import GithubWebhook +# Static default PR size thresholds: (threshold, label_name, color_hex) +STATIC_PR_SIZE_THRESHOLDS: tuple[tuple[int | float, str, str], ...] = ( + (20, "XS", "ededed"), + (50, "S", "0E8A16"), + (100, "M", "F09C74"), + (300, "L", "F5621C"), + (500, "XL", "D93F0B"), + (float("inf"), "XXL", "B60205"), +) + class LabelsHandler: def __init__(self, github_webhook: "GithubWebhook", owners_file_handler: OwnersFileHandler) -> None: @@ -38,6 +52,61 @@ def __init__(self, github_webhook: "GithubWebhook", owners_file_handler: OwnersF self.log_prefix: str = self.github_webhook.log_prefix self.repository: Repository = self.github_webhook.repository + def is_label_enabled(self, label: str) -> bool: + """Check if a label is enabled based on configuration. + + Args: + label: The label name or prefix to check. + + Returns: + True if the label is enabled, False otherwise. + + Note: + - If enabled_labels is None (not configured), all labels are enabled. + - reviewed-by labels (approved-*, lgtm-*, changes-requested-*, commented-*) + are always enabled and cannot be disabled. + - The exact "lgtm" and "approve" labels are always enabled and cannot be + disabled, as they are required for the review workflow. + """ + # reviewed-by labels are always enabled (cannot be disabled) + reviewed_by_prefixes = ( + APPROVED_BY_LABEL_PREFIX, + LGTM_BY_LABEL_PREFIX, + CHANGED_REQUESTED_BY_LABEL_PREFIX, + COMMENTED_BY_LABEL_PREFIX, + ) + if any(label.startswith(prefix) for prefix in reviewed_by_prefixes): + return True + + # Exact reviewed-by labels are also always enabled + if label in (LGTM_STR, APPROVE_STR): + return True + + enabled_labels = self.github_webhook.enabled_labels + + # If not configured, all labels are enabled + if enabled_labels is None: + return True + + # Check static labels using centralized category map + if label in LABEL_CATEGORY_MAP: + return LABEL_CATEGORY_MAP[label] in enabled_labels + + # Check size labels + if label.startswith(SIZE_LABEL_PREFIX): + return "size" in enabled_labels + + # Check branch labels + if label.startswith(BRANCH_LABEL_PREFIX): + return "branch" in enabled_labels + + # Check cherry-pick labels + if label.startswith(CHERRY_PICK_LABEL_PREFIX) or label == CHERRY_PICKED_LABEL: + return "cherry-pick" in enabled_labels + + # Unknown labels are allowed by default + return True + async def label_exists_in_pull_request(self, pull_request: PullRequest, label: str) -> bool: return label in await self.pull_request_labels_names(pull_request=pull_request) @@ -61,22 +130,27 @@ async def _remove_label(self, pull_request: PullRequest, label: str) -> bool: self.logger.debug(f"{self.log_prefix} Label {label} not found and cannot be removed") return False - async def _add_label(self, pull_request: PullRequest, label: str) -> None: + async def _add_label(self, pull_request: PullRequest, label: str) -> bool: + """Add a label to a pull request. + + Returns: + True if the label was added successfully, False if skipped. + """ label = label.strip() self.logger.debug(f"{self.log_prefix} Adding label {label}") if len(label) > 49: self.logger.debug(f"{label} is too long, not adding.") - return + return False - if await self.label_exists_in_pull_request(pull_request=pull_request, label=label): - self.logger.debug(f"{self.log_prefix} Label {label} already assign") - return + if not self.is_label_enabled(label): + self.logger.debug(f"{self.log_prefix} Label {label} is disabled by configuration, not adding") + return False - if label in STATIC_LABELS_DICT: - self.logger.info(f"{self.log_prefix} Adding pull request label {label}") - await asyncio.to_thread(pull_request.add_to_labels, label) - return + if await self.label_exists_in_pull_request(pull_request=pull_request, label=label): + self.logger.debug(f"{self.log_prefix} Label {label} already assigned") + return False + # Get the color for this label (custom or default) color = self._get_label_color(label) _with_color_msg = f"repository label {label} with color {color}" @@ -91,11 +165,12 @@ async def _add_label(self, pull_request: PullRequest, label: str) -> None: self.logger.info(f"{self.log_prefix} Adding pull request label {label}") await asyncio.to_thread(pull_request.add_to_labels, label) - await self.wait_for_label(pull_request=pull_request, label=label, exists=True) + return await self.wait_for_label(pull_request=pull_request, label=label, exists=True) async def wait_for_label(self, pull_request: PullRequest, label: str, exists: bool) -> bool: self.logger.debug(f"{self.log_prefix} waiting for label {label} to {'exists' if exists else 'not exists'}") - while TimeoutWatch(timeout=30).remaining_time() > 0: + timeout_watch = TimeoutWatch(timeout=30) + while timeout_watch.remaining_time() > 0: res = await self.label_exists_in_pull_request(pull_request=pull_request, label=label) if res == exists: return True @@ -108,28 +183,47 @@ async def wait_for_label(self, pull_request: PullRequest, label: str, exists: bo def _get_label_color(self, label: str) -> str: """Get the appropriate color for a label. + Checks configured colors first, then falls back to defaults. For size labels with custom thresholds, uses the custom color. - For other dynamic labels, uses the DYNAMIC_LABELS_DICT. - Falls back to default color if not found. """ + # Check for custom configured colors first + custom_colors = self.github_webhook.label_colors + # Handle misconfigured label_colors (must be dict, not list) + if not isinstance(custom_colors, dict): + custom_colors = {} + + # Direct match for static labels + if label in custom_colors: + return self._get_color_hex(custom_colors[label]) + + # Check prefix matches for dynamic labels + # First-match-wins: iteration order determines which prefix wins + # when multiple prefixes could match (e.g., "size-" vs "size-X") + for prefix, color in custom_colors.items(): + if prefix.endswith("-") and label.startswith(prefix): + return self._get_color_hex(color) + + # For size labels, check custom thresholds if label.startswith(SIZE_LABEL_PREFIX): size_name = label[len(SIZE_LABEL_PREFIX) :] - thresholds = self._get_custom_pr_size_thresholds() for _threshold, label_name, color_hex in thresholds: if label_name == size_name: return color_hex - - # If not found in custom thresholds, check static labels dict - # (for backward compatibility with static size labels) + # Fallback to STATIC_LABELS_DICT for default size labels if label in STATIC_LABELS_DICT: return STATIC_LABELS_DICT[label] - _color = [DYNAMIC_LABELS_DICT[_label] for _label in DYNAMIC_LABELS_DICT if _label in label] - if _color: - return _color[0] + # Check DEFAULT_LABEL_COLORS for static labels + if label in DEFAULT_LABEL_COLORS: + return DEFAULT_LABEL_COLORS[label] - return "D4C5F9" + # Check DEFAULT_LABEL_COLORS for dynamic label prefixes + for prefix, color in DEFAULT_LABEL_COLORS.items(): + if prefix.endswith("-") and label.startswith(prefix): + return color + + return "D4C5F9" # Default fallback color def _get_color_hex(self, color_name: str, default_color: str = "lightgray") -> str: """Convert CSS3 color name to hex value, with fallback to default.""" @@ -154,17 +248,26 @@ def _get_custom_pr_size_thresholds(self) -> list[tuple[int | float, str, str]]: custom_config = self.github_webhook.config.get_value("pr-size-thresholds", return_on_none=None) if not custom_config: - return [ - (20, "XS", "ededed"), - (50, "S", "0E8A16"), - (100, "M", "F09C74"), - (300, "L", "F5621C"), - (500, "XL", "D93F0B"), - (float("inf"), "XXL", "B60205"), - ] + return list(STATIC_PR_SIZE_THRESHOLDS) + + # Validate custom_config is a dict (could be misconfigured as list or other type) + if not isinstance(custom_config, dict): + self.logger.warning( + f"{self.log_prefix} pr-size-thresholds config is not a dict " + f"(got {type(custom_config).__name__}), using static defaults" + ) + return list(STATIC_PR_SIZE_THRESHOLDS) thresholds = [] for label_name, config in custom_config.items(): + # Validate each config entry is a dict + if not isinstance(config, dict): + self.logger.warning( + f"{self.log_prefix} pr-size-thresholds entry for '{label_name}' is not a dict " + f"(got {type(config).__name__}), skipping" + ) + continue + threshold = config.get("threshold") # Convert string "inf" to float("inf") for YAML compatibility @@ -186,16 +289,17 @@ def _get_custom_pr_size_thresholds(self) -> list[tuple[int | float, str, str]]: if not sorted_thresholds: self.logger.warning(f"{self.log_prefix} No valid custom thresholds found, using static defaults") - return self._get_custom_pr_size_thresholds() # Recursive call will return static defaults + # Return static defaults directly to avoid infinite recursion + return list(STATIC_PR_SIZE_THRESHOLDS) return sorted_thresholds - def get_size(self, pull_request: PullRequest) -> str: + async def get_size(self, pull_request: PullRequest) -> str: """Calculates size label based on additions and deletions.""" - - # Handle None values by defaulting to 0 - additions = pull_request.additions if pull_request.additions is not None else 0 - deletions = pull_request.deletions if pull_request.deletions is not None else 0 + additions, deletions = await asyncio.gather( + asyncio.to_thread(lambda: pull_request.additions), + asyncio.to_thread(lambda: pull_request.deletions), + ) size = additions + deletions self.logger.debug(f"{self.log_prefix} PR size is {size} (additions: {additions}, deletions: {deletions})") @@ -217,7 +321,7 @@ def get_size(self, pull_request: PullRequest) -> str: async def add_size_label(self, pull_request: PullRequest) -> None: """Add a size label to the pull request based on its additions and deletions.""" - size_label = self.get_size(pull_request=pull_request) + size_label = await self.get_size(pull_request=pull_request) self.logger.debug(f"{self.log_prefix} size label is {size_label}") if not size_label: self.logger.debug(f"{self.log_prefix} Size label not found") diff --git a/webhook_server/libs/handlers/owners_files_handler.py b/webhook_server/libs/handlers/owners_files_handler.py index 77f1e49d6..8e4409e90 100644 --- a/webhook_server/libs/handlers/owners_files_handler.py +++ b/webhook_server/libs/handlers/owners_files_handler.py @@ -107,6 +107,8 @@ async def list_changed_files(self, pull_request: PullRequest) -> list[str]: # Run git diff command on cloned repository # Quote clone_repo_dir to handle paths with spaces or special characters + # First try three-dot diff (shows changes since common ancestor) + diff_syntax = "..." # Track which syntax is used for accurate error reporting git_diff_command = ( f"git -C {shlex.quote(self.github_webhook.clone_repo_dir)} diff --name-only {base_sha}...{head_sha}" ) @@ -119,10 +121,24 @@ async def list_changed_files(self, pull_request: PullRequest) -> list[str]: mask_sensitive=self.github_webhook.mask_sensitive, ) + # If three-dot fails with "no merge base", try two-dot diff + if not success and "no merge base" in (err or "").lower(): + self.logger.warning(f"{self.log_prefix} No merge base found, falling back to two-dot diff") + diff_syntax = ".." # Update to reflect the fallback syntax + git_diff_command = ( + f"git -C {shlex.quote(self.github_webhook.clone_repo_dir)} diff --name-only {base_sha}..{head_sha}" + ) + success, out, err = await run_command( + command=git_diff_command, + log_prefix=self.log_prefix, + verify_stderr=False, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + # Check success flag - raise if git diff failed if not success: error_msg = ( - f"git diff command failed for {base_sha}...{head_sha}. " + f"git diff command failed for {base_sha}{diff_syntax}{head_sha}. " f"stdout: {out.strip() if out else '(empty)'}, " f"stderr: {err.strip() if err else '(empty)'}" ) @@ -145,7 +161,9 @@ async def list_changed_files(self, pull_request: PullRequest) -> list[str]: except Exception as ex: # Wrap unexpected exceptions with context - error_msg = f"Unexpected error getting changed files via git diff for {base_sha}...{head_sha}: {ex}" + error_msg = ( + f"Unexpected error getting changed files via git diff for {base_sha}{diff_syntax}{head_sha}: {ex}" + ) self.logger.exception(f"{self.log_prefix} {error_msg}") raise RuntimeError(error_msg) from ex diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 65880da27..2b4c05ca1 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -21,7 +21,7 @@ CAN_BE_MERGED_STR, CHANGED_REQUESTED_BY_LABEL_PREFIX, CHERRY_PICK_LABEL_PREFIX, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, COMMENTED_BY_LABEL_PREFIX, CONVENTIONAL_TITLE_STR, FAILURE_STR, @@ -219,7 +219,6 @@ async def set_wip_label_based_on_title(self, pull_request: PullRequest) -> None: def _prepare_welcome_comment(self) -> str: self.logger.info(f"{self.log_prefix} Prepare welcome comment") - supported_user_labels_str: str = "".join([f" * {label}\n" for label in USER_LABELS_DICT.keys()]) # Check if current user is auto-verified is_auto_verified = self.github_webhook.parent_committer in self.github_webhook.auto_verified_and_merged_users @@ -255,22 +254,17 @@ def _prepare_welcome_comment(self) -> str: {self._prepare_pre_commit_welcome_line}\ * **Branch Labeling**: Branch-specific labels are applied to track the target branch * **Auto-verification**: Auto-verified users have their PRs automatically marked as verified +{self._prepare_labels_config_welcome_section}\ ### 📋 Available Commands #### PR Status Management -* `/wip` - Mark PR as work in progress (adds WIP: prefix to title) -* `/wip cancel` - Remove work in progress status -* `/hold` - Block PR merging (approvers only) -* `/hold cancel` - Unblock PR merging -* `/verified` - Mark PR as verified -* `/verified cancel` - Remove verification status -* `/reprocess` - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed) +{self._prepare_pr_status_commands_section} #### Review & Approval * `/lgtm` - Approve changes (looks good to me) * `/approve` - Approve PR (approvers only) -* `/automerge` - Enable automatic merging when all requirements are met (maintainers and approvers only) +{self._prepare_automerge_command_line}\ * `/assign-reviewers` - Assign reviewers based on OWNERS file * `/assign-reviewer @username` - Assign specific reviewer * `/check-can-merge` - Check if PR meets merge requirements @@ -278,9 +272,7 @@ def _prepare_welcome_comment(self) -> str: #### Testing & Validation {self._prepare_retest_welcome_comment} {self._prepare_container_operations_welcome_section}\ -#### Cherry-pick Operations -* `/cherry-pick ` - Schedule cherry-pick to target branch when PR is merged - * Multiple branches: `/cherry-pick branch1 branch2 branch3` +{self._prepare_cherry_pick_section}\ #### Label Management * `/` - Add a label to the PR @@ -293,7 +285,7 @@ def _prepare_welcome_comment(self) -> str: 1. **Approval**: `/approve` from at least one approver 2. **LGTM Count**: Minimum {self.github_webhook.minimum_lgtm} `/lgtm` from reviewers 3. **Status Checks**: All required status checks must pass -4. **No Blockers**: No WIP, hold, or conflict labels +{self._prepare_no_blockers_requirement} 5. **Verified**: PR must be marked as verified (if verification is enabled) ### 📊 Review Process @@ -307,17 +299,12 @@ def _prepare_welcome_comment(self) -> str:
Available Labels -{supported_user_labels_str} +{self._prepare_available_labels_section}
### 💡 Tips -* **WIP Status**: Use `/wip` when your PR is not ready for review -* **Verification**: The verified label is automatically removed on each new commit -* **Cherry-picking**: Cherry-pick labels are processed when the PR is merged -* **Container Builds**: Container images are automatically tagged with the PR number -* **Permission Levels**: Some commands require approver permissions -* **Auto-verified Users**: Certain users have automatic verification and merge privileges +{self._prepare_tips_section} For more information, please refer to the project documentation or contact the maintainers. """ @@ -382,6 +369,137 @@ def _prepare_container_operations_welcome_section(self) -> str: """ return "\n" + @property + def _prepare_labels_config_welcome_section(self) -> str: + """Prepare the labels configuration section for the welcome comment.""" + enabled_labels = self.github_webhook.enabled_labels + + if enabled_labels is None: + return "* **Labels**: All label categories are enabled (default configuration)\n" + + if not enabled_labels: + return "* **Labels**: All configurable labels are disabled (only reviewed-by labels are active)\n" + + enabled_list = ", ".join(f"`{label}`" for label in sorted(enabled_labels)) + return f"* **Labels**: Enabled categories: {enabled_list}\n" + + @property + def _prepare_pr_status_commands_section(self) -> str: + """Prepare the PR Status Management commands section for the welcome comment. + + Only shows commands for enabled labels. + """ + commands: list[str] = [] + + if self.labels_handler.is_label_enabled(WIP_STR): + commands.append("* `/wip` - Mark PR as work in progress (adds WIP: prefix to title)") + commands.append("* `/wip cancel` - Remove work in progress status") + + if self.labels_handler.is_label_enabled(HOLD_LABEL_STR): + commands.append("* `/hold` - Block PR merging (approvers only)") + commands.append("* `/hold cancel` - Unblock PR merging") + + if self.labels_handler.is_label_enabled(VERIFIED_LABEL_STR): + commands.append("* `/verified` - Mark PR as verified") + commands.append("* `/verified cancel` - Remove verification status") + + # These commands are always available + commands.append( + "* `/reprocess` - Trigger complete PR workflow reprocessing " + "(useful if webhook failed or configuration changed)" + ) + commands.append("* `/regenerate-welcome` - Regenerate this welcome message") + + return "\n".join(commands) + + @property + def _prepare_available_labels_section(self) -> str: + """Prepare the Available Labels section for the welcome comment. + + Only shows labels that are enabled. + """ + enabled_user_labels = [ + label for label in USER_LABELS_DICT.keys() if self.labels_handler.is_label_enabled(label) + ] + + if not enabled_user_labels: + return "No configurable labels are enabled for this repository." + + return "".join([f" * {label}\n" for label in enabled_user_labels]) + + @property + def _prepare_tips_section(self) -> str: + """Prepare the Tips section for the welcome comment. + + Only shows tips for enabled labels. + """ + tips: list[str] = [] + + if self.labels_handler.is_label_enabled(WIP_STR): + tips.append("* **WIP Status**: Use `/wip` when your PR is not ready for review") + + if self.labels_handler.is_label_enabled(VERIFIED_LABEL_STR): + tips.append("* **Verification**: The verified label is automatically removed on each new commit") + + # Cherry-pick tip - check if cherry-pick labels are enabled + if self.labels_handler.is_label_enabled(CHERRY_PICKED_LABEL): + tips.append("* **Cherry-picking**: Cherry-pick labels are processed when the PR is merged") + + # Container builds tip - always shown if container builds are configured + if self.github_webhook.build_and_push_container: + tips.append("* **Container Builds**: Container images are automatically tagged with the PR number") + + # Permission and auto-verified tips are always relevant + tips.append("* **Permission Levels**: Some commands require approver permissions") + tips.append("* **Auto-verified Users**: Certain users have automatic verification and merge privileges") + + return "\n".join(tips) + + @property + def _prepare_no_blockers_requirement(self) -> str: + """Prepare the No Blockers merge requirement line. + + Only mentions labels that are enabled. + """ + blockers: list[str] = [] + + if self.labels_handler.is_label_enabled(WIP_STR): + blockers.append("WIP") + + if self.labels_handler.is_label_enabled(HOLD_LABEL_STR): + blockers.append("hold") + + # Conflict labels (has-conflicts) are always shown since they're fundamental + blockers.append("conflict") + + return f"4. **No Blockers**: No {', '.join(blockers)} labels" + + @property + def _prepare_automerge_command_line(self) -> str: + """Prepare the automerge command line for the welcome comment. + + Only shows the command if automerge is enabled. + """ + if self.labels_handler.is_label_enabled(AUTOMERGE_LABEL_STR): + return ( + "* `/automerge` - Enable automatic merging when all requirements are met " + "(maintainers and approvers only)\n" + ) + return "" + + @property + def _prepare_cherry_pick_section(self) -> str: + """Prepare the Cherry-pick Operations section for the welcome comment. + + Only shows the section if cherry-pick labels are enabled. + """ + if self.labels_handler.is_label_enabled(CHERRY_PICKED_LABEL): + return """#### Cherry-pick Operations +* `/cherry-pick ` - Schedule cherry-pick to target branch when PR is merged + * Multiple branches: `/cherry-pick branch1 branch2 branch3` +""" + return "" + async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None: """ Labels pull requests based on their mergeable state. @@ -881,7 +999,7 @@ async def _process_verified_for_update_or_new_pull_request(self, pull_request: P # Check if this is a cherry-picked PR labels = await asyncio.to_thread(lambda: list(pull_request.labels)) - is_cherry_picked = any(label.name == CHERRY_PICKED_LABEL_PREFIX for label in labels) + is_cherry_picked = any(label.name == CHERRY_PICKED_LABEL for label in labels) # If it's a cherry-picked PR and auto-verify is disabled for cherry-picks, skip auto-verification if is_cherry_picked and not self.github_webhook.auto_verify_cherry_picked_prs: @@ -1156,6 +1274,30 @@ def check_comments() -> bool: return await asyncio.to_thread(check_comments) + async def regenerate_welcome_message(self, pull_request: PullRequest) -> None: + """Regenerate and update the welcome message for this PR. + + If a welcome message exists, it will be updated. + If no welcome message exists, a new one will be created. + """ + welcome_msg = self._prepare_welcome_comment() + + def find_and_update_welcome_comment() -> bool: + """Find existing welcome comment and update it. Returns True if updated, False if not found.""" + for comment in pull_request.get_issue_comments(): + if self.github_webhook.issue_url_for_welcome_msg in comment.body: + comment.edit(body=welcome_msg) + return True + return False + + updated = await asyncio.to_thread(find_and_update_welcome_comment) + + if updated: + self.logger.info(f"{self.log_prefix} Updated existing welcome message") + else: + self.logger.info(f"{self.log_prefix} Creating new welcome message") + await asyncio.to_thread(pull_request.create_issue_comment, body=welcome_msg) + async def _tracking_issue_exists(self, pull_request: PullRequest) -> bool: """Check if tracking issue already exists for this PR.""" expected_body = self._generate_issue_body(pull_request=pull_request) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index cf468d245..6f6900f39 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -15,7 +15,7 @@ from webhook_server.utils import helpers as helpers_module from webhook_server.utils.constants import ( BUILD_CONTAINER_STR, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, PRE_COMMIT_STR, PREK_STR, @@ -487,7 +487,7 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie requested_by = reviewed_user or "by target-branch label" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") - new_branch_name = f"{CHERRY_PICKED_LABEL_PREFIX}-{pull_request.head.ref}-{shortuuid.uuid()[:5]}" + new_branch_name = f"{CHERRY_PICKED_LABEL}-{pull_request.head.ref}-{shortuuid.uuid()[:5]}" if not await self.is_branch_exists(branch=target_branch): err_msg = f"cherry-pick failed: {target_branch} does not exists" self.logger.error(err_msg) @@ -510,8 +510,8 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie f"{git_cmd} cherry-pick {commit_hash}", f"{git_cmd} push origin {new_branch_name}", f'bash -c "{hub_cmd} pull-request -b {target_branch} ' - f"-h {new_branch_name} -l {CHERRY_PICKED_LABEL_PREFIX} " - f"-m '{CHERRY_PICKED_LABEL_PREFIX}: [{target_branch}] " + f"-h {new_branch_name} -l {CHERRY_PICKED_LABEL} " + f"-m '{CHERRY_PICKED_LABEL}: [{target_branch}] " f"{commit_msg_striped}' -m 'cherry-pick {pull_request_url} " f"into {target_branch}' -m 'requested-by {requested_by}'\"", ] diff --git a/webhook_server/tests/test_app_utils.py b/webhook_server/tests/test_app_utils.py index deb5185bd..9cd2220f4 100644 --- a/webhook_server/tests/test_app_utils.py +++ b/webhook_server/tests/test_app_utils.py @@ -3,11 +3,14 @@ import datetime import hashlib import hmac +from datetime import UTC, timedelta +from unittest.mock import Mock, patch import pytest from fastapi import HTTPException -from webhook_server.utils.app_utils import format_duration, parse_datetime_string, verify_signature +from webhook_server.utils.app_utils import format_duration, log_webhook_summary, parse_datetime_string, verify_signature +from webhook_server.utils.context import WebhookContext class TestVerifySignature: @@ -132,3 +135,140 @@ def test_format_duration_hours_with_minutes(self) -> None: """Test format_duration with hours and remaining minutes.""" assert format_duration(3660000) == "1h1m" assert format_duration(5700000) == "1h35m" + + +class TestLogWebhookSummary: + """Test suite for log_webhook_summary function.""" + + def test_log_webhook_summary_with_complete_steps(self) -> None: + """Test log_webhook_summary with fully completed steps.""" + # Create context with completed_at set + start_time = datetime.datetime(2024, 1, 15, 10, 0, 0, tzinfo=UTC) + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx = WebhookContext( + hook_id="test-hook-1", + event_type="pull_request", + repository="owner/repo", + repository_full_name="owner/repo", + pr_number=42, + ) + + # Start and complete a step + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx.start_step("webhook_routing") + + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + timedelta(seconds=2) + ctx.complete_step("webhook_routing") + + # Set completed_at + ctx.completed_at = start_time + timedelta(seconds=5) + + # Mock logger + mock_logger = Mock() + + # Call the function - should not raise + log_webhook_summary(ctx, mock_logger, "[TEST]") + + # Verify logger was called with info level + mock_logger.info.assert_called_once() + log_message = mock_logger.info.call_args[0][0] + + # Verify log message contains expected information + assert "[SUCCESS]" in log_message + assert "PR#42" in log_message + assert "webhook_routing:completed(2s)" in log_message + + def test_log_webhook_summary_with_incomplete_step(self) -> None: + """Test log_webhook_summary handles incomplete steps (started but not completed). + + This tests the bug fix for: + ValueError: Workflow step 'webhook_routing' missing or None 'duration_ms' field + """ + # Create context + start_time = datetime.datetime(2024, 1, 15, 10, 0, 0, tzinfo=UTC) + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx = WebhookContext( + hook_id="test-hook-2", + event_type="pull_request", + repository="owner/repo", + repository_full_name="owner/repo", + ) + + # Start a step but DON'T complete it (simulating exception before complete_step) + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx.start_step("webhook_routing") + + # Set completed_at (happens in finally block even on exception) + ctx.completed_at = start_time + timedelta(seconds=3) + ctx.success = False + + # Mock logger + mock_logger = Mock() + + # Call the function - should NOT raise ValueError anymore + log_webhook_summary(ctx, mock_logger, "[TEST]") + + # Verify logger was called + mock_logger.info.assert_called_once() + log_message = mock_logger.info.call_args[0][0] + + # Verify incomplete step is shown as "(incomplete)" + assert "[FAILED]" in log_message + assert "webhook_routing:started(incomplete)" in log_message + + def test_log_webhook_summary_with_missing_status(self) -> None: + """Test log_webhook_summary handles steps with missing status field.""" + # Create context + start_time = datetime.datetime(2024, 1, 15, 10, 0, 0, tzinfo=UTC) + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx = WebhookContext( + hook_id="test-hook-3", + event_type="push", + repository="owner/repo", + repository_full_name="owner/repo", + ) + + # Manually add a malformed step (missing status) + ctx.workflow_steps["bad_step"] = {"timestamp": start_time.isoformat()} + + # Set completed_at + ctx.completed_at = start_time + timedelta(seconds=1) + + # Mock logger + mock_logger = Mock() + + # Call the function - should handle gracefully + log_webhook_summary(ctx, mock_logger, "[TEST]") + + # Verify logger was called + mock_logger.info.assert_called_once() + log_message = mock_logger.info.call_args[0][0] + + # Verify missing status defaults to "unknown" and shows as incomplete + assert "bad_step:unknown(incomplete)" in log_message + + def test_log_webhook_summary_raises_when_not_completed(self) -> None: + """Test log_webhook_summary raises ValueError when completed_at is None.""" + # Create context without setting completed_at + start_time = datetime.datetime(2024, 1, 15, 10, 0, 0, tzinfo=UTC) + with patch("webhook_server.utils.context.datetime") as mock_dt: + mock_dt.now.return_value = start_time + ctx = WebhookContext( + hook_id="test-hook-4", + event_type="push", + repository="owner/repo", + repository_full_name="owner/repo", + ) + + # Mock logger + mock_logger = Mock() + + # Call should raise ValueError because completed_at is None + with pytest.raises(ValueError, match="Context completed_at is None"): + log_webhook_summary(ctx, mock_logger, "[TEST]") diff --git a/webhook_server/tests/test_check_run_handler.py b/webhook_server/tests/test_check_run_handler.py index d420f9d24..bbdfe45ce 100644 --- a/webhook_server/tests/test_check_run_handler.py +++ b/webhook_server/tests/test_check_run_handler.py @@ -10,7 +10,7 @@ from webhook_server.utils.constants import ( BUILD_CONTAINER_STR, CAN_BE_MERGED_STR, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, FAILURE_STR, IN_PROGRESS_STR, @@ -381,7 +381,7 @@ async def test_set_cherry_pick_in_progress(self, check_run_handler: CheckRunHand """Test setting cherry pick check to in progress status.""" with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: await check_run_handler.set_cherry_pick_in_progress() - mock_set_status.assert_called_once_with(check_run=CHERRY_PICKED_LABEL_PREFIX, status=IN_PROGRESS_STR) + 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: @@ -390,7 +390,7 @@ async def test_set_cherry_pick_success(self, check_run_handler: CheckRunHandler) with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: await check_run_handler.set_cherry_pick_success(output) mock_set_status.assert_called_once_with( - check_run=CHERRY_PICKED_LABEL_PREFIX, conclusion=SUCCESS_STR, output=output + check_run=CHERRY_PICKED_LABEL, conclusion=SUCCESS_STR, output=output ) @pytest.mark.asyncio @@ -400,7 +400,7 @@ async def test_set_cherry_pick_failure(self, check_run_handler: CheckRunHandler) with patch.object(check_run_handler, "set_check_run_status") as mock_set_status: await check_run_handler.set_cherry_pick_failure(output) mock_set_status.assert_called_once_with( - check_run=CHERRY_PICKED_LABEL_PREFIX, conclusion=FAILURE_STR, output=output + check_run=CHERRY_PICKED_LABEL, conclusion=FAILURE_STR, output=output ) @pytest.mark.asyncio diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index d76947a7c..91e058bdf 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -641,3 +641,81 @@ def test_pr_size_thresholds_empty_configuration(self, valid_minimal_config: dict assert data["pr-size-thresholds"] == {} finally: shutil.rmtree(temp_dir) + + def test_labels_configuration_schema(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Test labels configuration at global and repository levels.""" + config_data = { + "log-level": "INFO", + "github-app-id": 123456, + "github-tokens": ["token1"], + "webhook-ip": "http://localhost:5000", + "labels": { + "enabled-labels": ["verified", "hold", "wip", "size"], + "colors": {"hold": "red", "verified": "green", "approved-": "blue"}, + }, + "repositories": { + "test-repo": { + "name": "org/test-repo", + "labels": { + "enabled-labels": ["verified", "size"], + "colors": {"hold": "orange"}, + }, + } + }, + } + + temp_dir = self.create_temp_config_dir_and_data(config_data) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config = Config() + # Verify global labels config + assert config.root_data["labels"]["enabled-labels"] == ["verified", "hold", "wip", "size"] + assert config.root_data["labels"]["colors"]["hold"] == "red" + # Verify repository labels config + assert config.root_data["repositories"]["test-repo"]["labels"]["enabled-labels"] == ["verified", "size"] + assert config.root_data["repositories"]["test-repo"]["labels"]["colors"]["hold"] == "orange" + finally: + shutil.rmtree(temp_dir) + + def test_labels_invalid_category_rejected(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Test that invalid label categories are rejected by schema validation.""" + config_data = { + "github-app-id": 123456, + "github-tokens": ["token1"], + "webhook-ip": "http://localhost:5000", + "labels": {"enabled-labels": ["invalid-category"]}, + "repositories": {"test-repo": {"name": "org/test-repo"}}, + } + + temp_dir = self.create_temp_config_dir_and_data(config_data) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + # Invalid label category should raise a validation error + with pytest.raises(ValueError, match=r"Invalid label categories in enabled-labels"): + Config() + finally: + shutil.rmtree(temp_dir) + + def test_labels_empty_enabled_labels(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Test that empty enabled-labels array is valid configuration.""" + config_data = { + "github-app-id": 123456, + "github-tokens": ["token1"], + "webhook-ip": "http://localhost:5000", + "labels": {"enabled-labels": []}, + "repositories": {"test-repo": {"name": "org/test-repo"}}, + } + + temp_dir = self.create_temp_config_dir_and_data(config_data) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config = Config() + assert config.root_data["labels"]["enabled-labels"] == [] + finally: + shutil.rmtree(temp_dir) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index f7e9c7795..e3f6c1581 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1,6 +1,7 @@ import asyncio import os import tempfile +from collections.abc import Awaitable, Callable from pathlib import Path from typing import Any from unittest.mock import AsyncMock, Mock, patch @@ -44,10 +45,10 @@ def pull_request_payload(self) -> dict[str, Any]: @pytest.fixture def push_payload(self) -> dict[str, Any]: - """Push webhook payload.""" + """Push webhook payload for tag push (the only push type that triggers cloning).""" return { "repository": {"name": "test-repo", "full_name": "my-org/test-repo"}, - "ref": "refs/heads/main", + "ref": "refs/tags/v1.0.0", "commits": [{"id": "abc123", "message": "Test commit", "author": {"name": "Test User"}}], } @@ -69,27 +70,27 @@ def minimal_hook_data(self) -> dict[str, Any]: } @pytest.fixture - def minimal_headers(self) -> dict[str, str]: - return {"X-GitHub-Event": "pull_request", "X-GitHub-Delivery": "abc"} + def minimal_headers(self) -> Headers: + return Headers({"X-GitHub-Event": "pull_request", "X-GitHub-Delivery": "abc"}) @pytest.fixture def logger(self): return get_logger(name="test") @pytest.fixture - def to_thread_sync(self) -> Any: + def to_thread_sync(self) -> Callable[..., Awaitable[object]]: """Async helper to make asyncio.to_thread awaitable while executing inline.""" - async def _to_thread_sync(fn: Any, *args: Any, **kwargs: Any) -> Any: + async def _to_thread_sync(fn: Callable[..., object], *args: object, **kwargs: object) -> object: return fn(*args, **kwargs) return _to_thread_sync @pytest.fixture - def get_value_side_effect(self) -> Any: + def get_value_side_effect(self) -> Callable[..., object]: """Side effect function for Config.get_value mock in clone tests.""" - def _get_value_side_effect(value: str, *_args: Any, **_kwargs: Any) -> Any: + def _get_value_side_effect(value: str, *_args: object, **_kwargs: object) -> bool | dict[str, object] | None: if value == "mask-sensitive-data": return True if value == "container": @@ -225,8 +226,7 @@ def test_process_ping_event( mock_get_repo_api.return_value = Mock() mock_get_app_api.return_value = Mock() mock_color.return_value = "test-repo" - headers = minimal_headers.copy() - headers["X-GitHub-Event"] = "ping" + headers = Headers({"X-GitHub-Event": "ping", "X-GitHub-Delivery": "abc"}) gh = GithubWebhook(minimal_hook_data, headers, logger) result = asyncio.run(gh.process()) assert result is None @@ -320,7 +320,7 @@ async def test_process_push_event( mock_get_repo: Mock, push_payload: dict[str, Any], ) -> None: - """Test processing push event.""" + """Test processing tag push event triggers cloning and PushHandler.""" # Mock GitHub API to prevent network calls mock_api = Mock() mock_api.rate_limiting = [100, 5000] @@ -736,15 +736,15 @@ def test_prepare_log_prefix_with_color_file( assert result2 is not None @pytest.mark.asyncio - async def test_process_check_run_event(self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock) -> None: + async def test_process_check_run_event(self) -> None: """Test processing check run event.""" + logger = Mock() check_run_data = { "action": "completed", "repository": {"name": "test-repo", "full_name": "org/test-repo"}, "check_run": {"name": "test-check", "head_sha": "abc123", "status": "completed", "conclusion": "success"}, } - headers = minimal_headers.copy() - headers["X-GitHub-Event"] = "check_run" + headers = Headers({"X-GitHub-Event": "check_run", "X-GitHub-Delivery": "abc"}) with tempfile.TemporaryDirectory() as temp_dir: with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -812,7 +812,7 @@ async def test_process_check_run_event(self, minimal_hook_data: dict, minimal_he @pytest.mark.asyncio async def test_get_pull_request_by_number( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test getting pull request by number.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -842,7 +842,7 @@ async def test_get_pull_request_by_number( @pytest.mark.asyncio async def test_get_pull_request_github_exception( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test getting pull request with GithubException.""" @@ -870,9 +870,7 @@ async def test_get_pull_request_github_exception( assert result is None @pytest.mark.asyncio - async def test_get_pull_request_by_commit_with_pulls( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock - ) -> None: + async def test_get_pull_request_by_commit_with_pulls(self, minimal_headers: Headers, logger: Mock) -> None: """Test getting pull request by commit with pulls.""" commit_data = { "repository": {"name": "test-repo", "full_name": "my-org/test-repo"}, @@ -907,7 +905,7 @@ async def test_get_pull_request_by_commit_with_pulls( assert result == mock_pr def test_container_repository_and_tag_with_tag( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test container_repository_and_tag with provided tag.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -933,7 +931,7 @@ def test_container_repository_and_tag_with_tag( assert result == "test-repo:v1.0.0" def test_container_repository_and_tag_with_pull_request( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test container_repository_and_tag with pull request.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -962,7 +960,7 @@ def test_container_repository_and_tag_with_pull_request( assert result == "test-repo:pr-123" def test_container_repository_and_tag_merged_pr( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test container_repository_and_tag with merged pull request.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -992,7 +990,7 @@ def test_container_repository_and_tag_merged_pr( assert result == "test-repo:develop" def test_container_repository_and_tag_no_pull_request( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test container_repository_and_tag without pull request.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1017,7 +1015,7 @@ def test_container_repository_and_tag_no_pull_request( assert result is None def test_current_pull_request_supported_retest_property( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test _current_pull_request_supported_retest property.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1053,7 +1051,7 @@ def test_current_pull_request_supported_retest_property( assert "conventional-title" in result @pytest.mark.asyncio - async def test_get_last_commit(self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock) -> None: + async def test_get_last_commit(self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock) -> None: """Test _get_last_commit method.""" with patch("webhook_server.libs.github_api.Config") as mock_config: mock_config.return_value.repository = True @@ -1084,10 +1082,10 @@ async def test_get_last_commit(self, minimal_hook_data: dict, minimal_headers: d async def test_clone_repository_success( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, - get_value_side_effect: Any, - to_thread_sync: Any, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test successful repository clone for PR.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1121,7 +1119,7 @@ async def test_clone_repository_success( mock_pr.base = mock_base # Mock run_command to succeed for all git operations - async def mock_run_command(*_args: Any, **_kwargs: Any) -> tuple[bool, str, str]: + async def mock_run_command(*_args: object, **_kwargs: object) -> tuple[bool, str, str]: return (True, "", "") with ( @@ -1135,7 +1133,7 @@ async def mock_run_command(*_args: Any, **_kwargs: Any) -> tuple[bool, str, str] @pytest.mark.asyncio async def test_clone_repository_already_cloned( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test early return when repository already cloned.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1169,9 +1167,9 @@ async def test_clone_repository_already_cloned( async def test_clone_repository_clone_failure( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, - get_value_side_effect: Any, + get_value_side_effect: Callable[..., object], ) -> None: """Test RuntimeError raised when git clone fails.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1198,7 +1196,10 @@ async def test_clone_repository_clone_failure( mock_pr = Mock() # Mock run_command to fail on clone - async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str]: + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: + del log_prefix # unused if "git clone" in command: return (False, "", "Permission denied") return (True, "", "") @@ -1213,10 +1214,10 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str async def test_clone_repository_checkout_failure( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, - get_value_side_effect: Any, - to_thread_sync: Any, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test RuntimeError raised when git checkout fails.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1250,8 +1251,10 @@ async def test_clone_repository_checkout_failure( mock_pr.base = mock_base # Mock run_command: succeed for clone/config, fail for checkout - async def mock_run_command(**kwargs: Any) -> tuple[bool, str, str]: - command = kwargs.get("command", "") + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: + del log_prefix # unused if "checkout main" in command: return (False, "", "Branch not found") return (True, "", "") @@ -1267,10 +1270,10 @@ async def mock_run_command(**kwargs: Any) -> tuple[bool, str, str]: async def test_clone_repository_git_config_warnings( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, - get_value_side_effect: Any, - to_thread_sync: Any, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test that git config failures log warnings but don't raise exceptions.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1302,14 +1305,15 @@ async def test_clone_repository_git_config_warnings( mock_base = Mock() mock_base.ref = "main" mock_pr.base = mock_base + mock_pr.number = 123 - # Mock run_command: succeed for clone/checkout, fail for config commands - async def mock_run_command(**kwargs: Any) -> tuple[bool, str, str]: - command = kwargs.get("command", "") + # Mock run_command: succeed for clone/checkout, fail for config commands only + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: + del log_prefix # unused if "config user.name" in command or "config user.email" in command: return (False, "", "Config failed") - if "config --local --add" in command or "remote update" in command: - return (False, "", "Config failed") return (True, "", "") mock_logger = Mock() @@ -1326,15 +1330,83 @@ async def mock_run_command(**kwargs: Any) -> tuple[bool, str, str]: # Verify warnings were logged for each config failure warning_calls = [call for call in mock_logger.warning.call_args_list] - assert len(warning_calls) == 4 # user.name, user.email, fetch config, remote update + assert len(warning_calls) == 2 # user.name, user.email + + @pytest.mark.asyncio + async def test_clone_repository_pr_ref_fetch_failure_raises( + self, + minimal_hook_data: dict, + minimal_headers: Headers, + logger: Mock, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], + ) -> None: + """Test that PR ref fetch failure raises RuntimeError (fatal error).""" + 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_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-token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_repo = Mock() + mock_repo.clone_url = "https://github.com/org/test-repo.git" + mock_owner = Mock() + mock_owner.login = "test-owner" + mock_repo.owner = mock_owner + mock_get_repo_api.return_value = mock_repo + + with patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_get_app_api: + mock_get_app_api.return_value = Mock() + + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") as mock_color: + mock_color.return_value = "test-repo" + + gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) + + # Mock pull request + mock_pr = Mock() + mock_base = Mock() + mock_base.ref = "main" + mock_pr.base = mock_base + mock_pr.number = 456 + + # Mock run_command: succeed for clone, fail for PR ref fetch + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: + del log_prefix # unused + if "fetch origin +refs/pull/" in command: + return (False, "", "Fetch failed: PR ref not found") + return (True, "", "") + + mock_logger = Mock() + + with ( + patch("webhook_server.libs.github_api.run_command", side_effect=mock_run_command), + patch("asyncio.to_thread", side_effect=to_thread_sync), + patch.object(gh, "logger", mock_logger), + ): + with pytest.raises(RuntimeError) as exc_info: + await gh._clone_repository(pull_request=mock_pr) + + # Verify error message contains PR number and error + assert "456" in str(exc_info.value) + assert "Failed to fetch PR" in str(exc_info.value) + + # Verify error was logged + error_calls = list(mock_logger.error.call_args_list) + assert len(error_calls) >= 1 @pytest.mark.asyncio async def test_clone_repository_general_exception( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, - get_value_side_effect: Any, + get_value_side_effect: Callable[..., object], ) -> None: """Test exception handling during clone operation.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1361,7 +1433,7 @@ async def test_clone_repository_general_exception( mock_pr = Mock() # Mock run_command to raise an exception - async def mock_run_command(*_args: Any, **_kwargs: Any) -> tuple[bool, str, str]: + async def mock_run_command(*_args: object, **_kwargs: object) -> tuple[bool, str, str]: raise ValueError("Unexpected error during git operation") with ( @@ -1372,7 +1444,7 @@ async def mock_run_command(*_args: Any, **_kwargs: Any) -> tuple[bool, str, str] @pytest.mark.asyncio async def test_clone_repository_no_arguments( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock ) -> None: """Test _clone_repository raises ValueError when no arguments provided.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1401,7 +1473,7 @@ async def test_clone_repository_no_arguments( async def test_clone_repository_empty_checkout_ref( self, minimal_hook_data: dict, - minimal_headers: dict, + minimal_headers: Headers, logger: Mock, tmp_path: Path, ) -> None: @@ -1431,7 +1503,7 @@ async def test_clone_repository_empty_checkout_ref( # Create webhook webhook = GithubWebhook( hook_data=minimal_hook_data, - headers=Headers(minimal_headers), + headers=minimal_headers, logger=logger, ) @@ -1439,6 +1511,184 @@ async def test_clone_repository_empty_checkout_ref( with pytest.raises(ValueError, match="requires either pull_request or checkout_ref"): await webhook._clone_repository(checkout_ref="") + @pytest.mark.asyncio + async def test_clone_repository_checkout_ref_fetch_path_for_tag( + self, + minimal_hook_data: dict, + minimal_headers: Headers, + logger: Mock, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], + ) -> None: + """Test _clone_repository with checkout_ref fetches and checks out the correct tag. + + Note: checkout_ref is now only used for tags, as branch pushes skip cloning. + + Verifies that when checkout_ref="refs/tags/v1.0.0" is provided: + 1. git fetch origin refs/tags/v1.0.0:refs/tags/v1.0.0 is called + 2. git checkout v1.0.0 is called + """ + 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_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-token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_repo = Mock() + mock_repo.clone_url = "https://github.com/org/test-repo.git" + mock_owner = Mock() + mock_owner.login = "test-owner" + mock_repo.owner = mock_owner + mock_get_repo_api.return_value = mock_repo + + with patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_get_app_api: + mock_get_app_api.return_value = Mock() + + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") as mock_color: + mock_color.return_value = "test-repo" + + gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) + + # Track commands executed + executed_commands: list[str] = [] + + async def mock_run_command(command: str, **_kwargs: object) -> tuple[bool, str, str]: + executed_commands.append(command) + return (True, "", "") + + with ( + patch( + "webhook_server.libs.github_api.run_command", + side_effect=mock_run_command, + ), + patch("asyncio.to_thread", side_effect=to_thread_sync), + ): + await gh._clone_repository(checkout_ref="refs/tags/v1.0.0") + + # Verify clone succeeded + assert gh._repo_cloned is True + + # Verify tag fetch command contains the expected refspec + # Using flexible matching to tolerate added git flags + tag_refspec = "refs/tags/v1.0.0:refs/tags/v1.0.0" + fetch_commands = [cmd for cmd in executed_commands if tag_refspec in cmd] + assert len(fetch_commands) == 1, ( + f"Expected exactly one fetch command containing refspec '{tag_refspec}', " + f"got: {fetch_commands}" + ) + + # Verify checkout command contains the tag name + checkout_commands = [ + cmd for cmd in executed_commands if "checkout" in cmd and "v1.0.0" in cmd + ] + assert len(checkout_commands) == 1, ( + f"Expected exactly one checkout command for v1.0.0, got: {checkout_commands}" + ) + + @pytest.mark.asyncio + async def test_clone_repository_fetches_base_branch_for_pr( + self, + minimal_hook_data: dict, + minimal_headers: Headers, + logger: Mock, + get_value_side_effect: Callable[..., object], + to_thread_sync: Callable[..., Awaitable[object]], + ) -> None: + """Test _clone_repository fetches base branch before PR ref when pull_request is provided. + + Verifies that when _clone_repository is called with a pull_request: + 1. git fetch origin {base_ref} is called first (base branch fetch) + 2. git fetch origin +refs/pull/{pr_number}/head:refs/remotes/origin/pr/{pr_number} is called + 3. The base branch fetch happens BEFORE the PR ref fetch + """ + 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_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-token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_repo = Mock() + mock_repo.clone_url = "https://github.com/org/test-repo.git" + mock_owner = Mock() + mock_owner.login = "test-owner" + mock_repo.owner = mock_owner + mock_get_repo_api.return_value = mock_repo + + with patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_get_app_api: + mock_get_app_api.return_value = Mock() + + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") as mock_color: + mock_color.return_value = "test-repo" + + gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) + + # Mock pull request with base.ref = "release-1.0" and number = 123 + mock_pr = Mock() + mock_base = Mock() + mock_base.ref = "release-1.0" + mock_pr.base = mock_base + mock_pr.number = 123 + + # Track commands executed in order + executed_commands: list[str] = [] + + async def mock_run_command(command: str, **_kwargs: object) -> tuple[bool, str, str]: + executed_commands.append(command) + return (True, "", "") + + with ( + patch( + "webhook_server.libs.github_api.run_command", + side_effect=mock_run_command, + ), + patch("asyncio.to_thread", side_effect=to_thread_sync), + ): + await gh._clone_repository(pull_request=mock_pr) + + # Verify clone succeeded + assert gh._repo_cloned is True + + # Verify base branch fetch command contains the branch name + # Using flexible matching to tolerate added git flags + base_branch = "release-1.0" + base_fetch_commands = [ + cmd + for cmd in executed_commands + if "fetch" in cmd and base_branch in cmd and "refs/pull" not in cmd + ] + assert len(base_fetch_commands) == 1, ( + f"Expected exactly one fetch command for base branch '{base_branch}', " + f"got: {base_fetch_commands}" + ) + + # Verify PR ref fetch command contains the expected refspec + pr_refspec = "+refs/pull/123/head:refs/remotes/origin/pr/123" + pr_fetch_commands = [cmd for cmd in executed_commands if pr_refspec in cmd] + assert len(pr_fetch_commands) == 1, ( + f"Expected exactly one PR ref fetch command containing '{pr_refspec}', " + f"got: {pr_fetch_commands}" + ) + + # Verify order: base branch fetch should come BEFORE PR ref fetch + # Use index into executed_commands for ordering check + base_fetch_index = next( + i + for i, cmd in enumerate(executed_commands) + if "fetch" in cmd and base_branch in cmd and "refs/pull" not in cmd + ) + pr_fetch_index = next(i for i, cmd in enumerate(executed_commands) if pr_refspec in cmd) + assert base_fetch_index < pr_fetch_index, ( + f"Base branch fetch (index {base_fetch_index}) should come before " + f"PR ref fetch (index {pr_fetch_index}). " + f"Commands: {executed_commands}" + ) + @patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"}) @patch("webhook_server.libs.github_api.get_github_repo_api") @patch("webhook_server.libs.github_api.get_repository_github_app_api") @@ -1525,7 +1775,7 @@ async def test_process_push_event_deletion( @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") - async def test_process_push_event_normal_push_not_deletion( + async def test_process_push_event_branch_push_skips_clone( self, mock_auto_verified_prop: Mock, mock_repo_local_data: Mock, @@ -1535,18 +1785,21 @@ async def test_process_push_event_normal_push_not_deletion( mock_repo_api: Mock, mock_get_repo: Mock, ) -> None: - """Test that normal push event (deleted=False) proceeds with normal processing. + """Test that branch push event skips cloning. + + Branch pushes don't require cloning because PushHandler only + processes tags (PyPI upload, container build). Verifies: - - Processing continues when hook_data["deleted"] == False - - _clone_repository IS called - - PushHandler.process_push_webhook_data IS called + - _clone_repository is NOT called for branch pushes + - PushHandler.process_push_webhook_data is NOT called + - Returns None """ - # Normal push payload (no deletion) - push_normal_payload = { + # Branch push payload + push_branch_payload = { "repository": {"name": "test-repo", "full_name": "my-org/test-repo"}, "ref": "refs/heads/main", - "deleted": False, # Explicitly not a deletion + "deleted": False, "commits": [{"id": "abc123", "message": "Test commit", "author": {"name": "Test User"}}], } @@ -1564,26 +1817,32 @@ async def test_process_push_event_normal_push_not_deletion( mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER") mock_repo_api.return_value = Mock() mock_get_repo.return_value = mock_repository - mock_get_apis.return_value = [] # Return empty list to skip the problematic property code + mock_get_apis.return_value = [] mock_repo_local_data.return_value = {} - mock_process_push.return_value = None - headers = Headers({"X-GitHub-Event": "push", "X-GitHub-Delivery": "test-normal-push-456"}) - webhook = GithubWebhook(hook_data=push_normal_payload, headers=headers, logger=Mock()) + headers = Headers({"X-GitHub-Event": "push", "X-GitHub-Delivery": "test-branch-push-456"}) + mock_logger = Mock() + webhook = GithubWebhook(hook_data=push_branch_payload, headers=headers, logger=mock_logger) - # Mock _clone_repository to verify it IS called for normal pushes + # Mock _clone_repository to verify it is NOT called for branch pushes with patch.object(webhook, "_clone_repository", new=AsyncMock(return_value=None)) as mock_clone: - await webhook.process() + result = await webhook.process() - # Verify _clone_repository WAS called (normal push should clone) - mock_clone.assert_called_once_with(checkout_ref="refs/heads/main") + # Verify _clone_repository was NOT called (branch push skips cloning) + mock_clone.assert_not_called() - # Verify PushHandler.process_push_webhook_data was called - mock_process_push.assert_called_once() + # Verify PushHandler.process_push_webhook_data was NOT called + mock_process_push.assert_not_called() + + # Verify return value is None + assert result is None @pytest.mark.asyncio async def test_cleanup( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + self, + minimal_hook_data: dict, + minimal_headers: Headers, + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method removes temporary directory.""" mock_logger = Mock() @@ -1612,7 +1871,10 @@ async def test_cleanup( @pytest.mark.asyncio async def test_cleanup_exception( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + self, + minimal_hook_data: dict, + minimal_headers: Headers, + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method handles exceptions.""" mock_logger = Mock() @@ -1640,3 +1902,66 @@ def rmtree_fail(*args, **kwargs): await gh.cleanup() mock_logger.warning.assert_called() + + @patch("webhook_server.libs.github_api.Config") + @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") + @patch("webhook_server.libs.github_api.get_github_repo_api") + @patch("webhook_server.libs.github_api.get_repository_github_app_api") + @patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") + def test_enabled_labels_with_non_string_entries_logs_warning( + self, + mock_color: Mock, + mock_get_app_api: Mock, + mock_get_repo_api: Mock, + mock_get_api: Mock, + mock_config: Mock, + minimal_hook_data: dict, + minimal_headers: Headers, + ) -> None: + """Test that non-string entries in enabled-labels are sanitized and logged.""" + mock_logger = Mock() + + # Configure mock to return enabled-labels with non-string items + def get_value_side_effect(value: str, *_args: object, **kwargs: object) -> object: + if value == "labels": + # Return labels config with non-string entries in enabled-labels + return { + "enabled-labels": [ + "verified", # Valid string - valid category + {"key1": "val1", "key2": "val2"}, # Dict - should be sanitized + ["nested", "list"], # List - should be sanitized + 12345, # Integer - should be sanitized + ] + } + if value == "mask-sensitive-data": + return True + return kwargs.get("return_on_none") + + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + mock_config.return_value.get_value.side_effect = get_value_side_effect + mock_get_api.return_value = (Mock(), "token", "apiuser") + mock_get_repo_api.return_value = Mock() + mock_get_app_api.return_value = Mock() + mock_color.return_value = "test-repo" + + gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) + + # Verify warning was logged about non-string entries + mock_logger.warning.assert_called() + # Search through all warning calls for the non-string entries warning + warning_calls = [call[0][0] for call in mock_logger.warning.call_args_list] + non_string_warning = next( + (msg for msg in warning_calls if "Non-string entries in enabled-labels were ignored" in msg), + None, + ) + assert non_string_warning is not None, ( + f"Expected warning about non-string entries not found in: {warning_calls}" + ) + assert "dict(keys=" in non_string_warning + assert "list(len=2)" in non_string_warning + assert "int(" in non_string_warning + + # 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 diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 88185efe4..28d8506ab 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -11,6 +11,7 @@ COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REGENERATE_WELCOME_STR, COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, HOLD_LABEL_STR, @@ -423,6 +424,72 @@ async def test_user_commands_wip_remove(self, issue_comment_handler: IssueCommen assert called_args["title"].strip() == "Test PR" mock_reaction.assert_called_once() + @pytest.mark.asyncio + async def test_user_commands_wip_add_idempotent(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test that adding WIP when title already has WIP: prefix does not prepend again.""" + mock_pull_request = Mock() + mock_pull_request.title = "WIP: Test PR" + + with patch.object(issue_comment_handler, "create_comment_reaction") as mock_reaction: + with patch.object( + issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock + ) as mock_add_label: + mock_add_label.return_value = True # Label was added (or already existed) + with patch.object(mock_pull_request, "edit") as mock_edit: + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, command=WIP_STR, reviewed_user="test-user", issue_comment_id=123 + ) + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=WIP_STR) + # Should NOT edit title since it already starts with WIP: + mock_edit.assert_not_called() + mock_reaction.assert_called_once() + + @pytest.mark.asyncio + async def test_user_commands_wip_remove_no_prefix(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test that removing WIP when title has no WIP: prefix does not edit title.""" + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" # No WIP: prefix + + with patch.object(issue_comment_handler, "create_comment_reaction") as mock_reaction: + with patch.object( + issue_comment_handler.labels_handler, "_remove_label", new_callable=AsyncMock + ) as mock_remove_label: + mock_remove_label.return_value = True # Label was removed + with patch.object(mock_pull_request, "edit") as mock_edit: + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{WIP_STR} cancel", + reviewed_user="test-user", + issue_comment_id=123, + ) + mock_remove_label.assert_called_once_with(pull_request=mock_pull_request, label=WIP_STR) + # Should NOT edit title since it doesn't start with WIP: + mock_edit.assert_not_called() + mock_reaction.assert_called_once() + + @pytest.mark.asyncio + async def test_user_commands_wip_remove_no_space(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test removing WIP when title has WIP: prefix without space after colon.""" + mock_pull_request = Mock() + mock_pull_request.title = "WIP:Test PR" # No space after colon + + with patch.object(issue_comment_handler, "create_comment_reaction") as mock_reaction: + with patch.object( + issue_comment_handler.labels_handler, "_remove_label", new_callable=AsyncMock + ) as mock_remove_label: + mock_remove_label.return_value = True # Label was removed + with patch.object(mock_pull_request, "edit") as mock_edit: + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{WIP_STR} cancel", + reviewed_user="test-user", + issue_comment_id=123, + ) + mock_remove_label.assert_called_once_with(pull_request=mock_pull_request, label=WIP_STR) + # Should edit title to remove WIP: (without space) + mock_edit.assert_called_once_with(title="Test PR") + mock_reaction.assert_called_once() + @pytest.mark.asyncio async def test_user_commands_hold_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: """Test user commands with hold command by unauthorized user.""" @@ -441,47 +508,47 @@ async def test_user_commands_hold_unauthorized_user(self, issue_comment_handler: @pytest.mark.asyncio async def test_user_commands_hold_authorized_user_add(self, issue_comment_handler: IssueCommentHandler) -> None: - """Test user commands with hold command by authorized user to add.""" + """Test user commands with hold command by authorized user to add. + + Note: check_if_can_be_merged is NOT called directly here - it's triggered + by the 'labeled' webhook event (hook-driven architecture). + """ mock_pull_request = Mock() with patch.object(issue_comment_handler, "create_comment_reaction") as mock_reaction: with patch.object( issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock ) as mock_add_label: - with patch.object( - issue_comment_handler.pull_request_handler, "check_if_can_be_merged", new_callable=AsyncMock - ) as mock_check: - await issue_comment_handler.user_commands( - pull_request=mock_pull_request, - command=HOLD_LABEL_STR, - reviewed_user="approver1", - issue_comment_id=123, - ) - mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HOLD_LABEL_STR) - mock_check.assert_called_once_with(pull_request=mock_pull_request) - mock_reaction.assert_called_once() + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=HOLD_LABEL_STR, + reviewed_user="approver1", + issue_comment_id=123, + ) + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HOLD_LABEL_STR) + mock_reaction.assert_called_once() @pytest.mark.asyncio async def test_user_commands_hold_authorized_user_remove(self, issue_comment_handler: IssueCommentHandler) -> None: - """Test user commands with hold command by authorized user to remove.""" + """Test user commands with hold command by authorized user to remove. + + Note: check_if_can_be_merged is NOT called directly here - it's triggered + by the 'unlabeled' webhook event (hook-driven architecture). + """ mock_pull_request = Mock() with patch.object(issue_comment_handler, "create_comment_reaction") as mock_reaction: with patch.object( issue_comment_handler.labels_handler, "_remove_label", new_callable=AsyncMock ) as mock_remove_label: - with patch.object( - issue_comment_handler.pull_request_handler, "check_if_can_be_merged", new_callable=AsyncMock - ) as mock_check: - await issue_comment_handler.user_commands( - pull_request=mock_pull_request, - command=f"{HOLD_LABEL_STR} cancel", - reviewed_user="approver1", - issue_comment_id=123, - ) - mock_remove_label.assert_called_once_with(pull_request=mock_pull_request, label=HOLD_LABEL_STR) - mock_check.assert_called_once_with(pull_request=mock_pull_request) - mock_reaction.assert_called_once() + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{HOLD_LABEL_STR} cancel", + reviewed_user="approver1", + issue_comment_id=123, + ) + mock_remove_label.assert_called_once_with(pull_request=mock_pull_request, label=HOLD_LABEL_STR) + mock_reaction.assert_called_once() @pytest.mark.asyncio async def test_user_commands_verified_add(self, issue_comment_handler: IssueCommentHandler) -> None: @@ -940,3 +1007,70 @@ async def test_user_commands_reprocess_reaction_added(self, issue_comment_handle mock_reaction.assert_awaited_once_with( pull_request=mock_pull_request, issue_comment_id=456, reaction=REACTIONS.ok ) + + @pytest.mark.asyncio + async def test_user_commands_regenerate_welcome_command_registration( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test that regenerate-welcome command is in available_commands list.""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.pull_request_handler, "regenerate_welcome_message", new=AsyncMock() + ) as mock_regenerate, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REGENERATE_WELCOME_STR, + reviewed_user="test-user", + issue_comment_id=123, + ) + # Command should be recognized and processed + mock_regenerate.assert_awaited_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_user_commands_regenerate_welcome_with_reaction( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test that reaction is added to comment for regenerate-welcome command.""" + mock_pull_request = Mock() + + with ( + patch.object(issue_comment_handler.pull_request_handler, "regenerate_welcome_message", new=AsyncMock()), + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()) as mock_reaction, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REGENERATE_WELCOME_STR, + reviewed_user="test-user", + issue_comment_id=456, + ) + # Verify reaction was added with correct comment ID and reaction type + mock_reaction.assert_awaited_once_with( + pull_request=mock_pull_request, issue_comment_id=456, reaction=REACTIONS.ok + ) + + @pytest.mark.asyncio + async def test_user_commands_regenerate_welcome_with_args_ignored( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test regenerate-welcome command ignores additional arguments.""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.pull_request_handler, "regenerate_welcome_message", new=AsyncMock() + ) as mock_regenerate, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + # Command with args (should be processed but args ignored) + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_REGENERATE_WELCOME_STR} some-args", + reviewed_user="test-user", + issue_comment_id=123, + ) + # Verify regenerate was called (args are ignored) + mock_regenerate.assert_awaited_once_with(pull_request=mock_pull_request) diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index a8c2df4b7..8f0096575 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -9,16 +9,21 @@ from webhook_server.utils.constants import ( ADD_STR, APPROVE_STR, + AUTOMERGE_LABEL_STR, + BRANCH_LABEL_PREFIX, + CHERRY_PICK_LABEL_PREFIX, + CHERRY_PICKED_LABEL, HOLD_LABEL_STR, LGTM_STR, SIZE_LABEL_PREFIX, STATIC_LABELS_DICT, + VERIFIED_LABEL_STR, WIP_STR, ) class MockPullRequest: - def __init__(self, additions: int | None = 50, deletions: int | None = 10): + def __init__(self, additions: int = 50, deletions: int = 10) -> None: self.additions = additions self.deletions = deletions self.number = 123 @@ -51,6 +56,10 @@ def mock_github_webhook(self) -> Mock: # This ensures existing tests use static defaults webhook.config.get_value.return_value = None webhook.ctx = None + # enabled_labels: None means all labels are enabled + webhook.enabled_labels = None + # label_colors: empty dict means use defaults + webhook.label_colors = {} return webhook @pytest.fixture @@ -70,6 +79,7 @@ def mock_pull_request(self) -> Mock: """Mock pull request object.""" return Mock(spec=PullRequest) + @pytest.mark.asyncio @pytest.mark.parametrize( "additions,deletions,expected_size", [ @@ -82,7 +92,7 @@ def mock_pull_request(self) -> Mock: (600, 400, "XXL"), # Extra extra large changes (500+ total) ], ) - def test_get_size_calculation( + async def test_get_size_calculation( self, labels_handler: LabelsHandler, additions: int, deletions: int, expected_size: str ) -> None: """Test pull request size calculation with various line counts.""" @@ -90,40 +100,10 @@ def test_get_size_calculation( pull_request.additions = additions pull_request.deletions = deletions - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == f"{SIZE_LABEL_PREFIX}{expected_size}" - def test_get_size_none_additions(self, labels_handler: LabelsHandler) -> None: - """Test size calculation when additions is None.""" - pull_request = Mock(spec=PullRequest) - pull_request.additions = None - pull_request.deletions = 10 - - result = labels_handler.get_size(pull_request=pull_request) - - assert result.startswith(SIZE_LABEL_PREFIX) - - def test_get_size_none_deletions(self, labels_handler: LabelsHandler) -> None: - """Test size calculation when deletions is None.""" - pull_request = Mock(spec=PullRequest) - pull_request.additions = 50 - pull_request.deletions = None - - result = labels_handler.get_size(pull_request=pull_request) - - assert result.startswith(SIZE_LABEL_PREFIX) - - def test_get_size_both_none(self, labels_handler: LabelsHandler) -> None: - """Test size calculation when both additions and deletions are None.""" - pull_request = Mock(spec=PullRequest) - pull_request.additions = None - pull_request.deletions = None - - result = labels_handler.get_size(pull_request=pull_request) - - assert result == f"{SIZE_LABEL_PREFIX}XS" - @pytest.mark.asyncio async def test_add_label_success(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: """Test successful label addition.""" @@ -152,14 +132,43 @@ async def test_add_label_already_exists(self, labels_handler: LabelsHandler, moc # Verify label was not added (already exists) mock_pull_request.add_to_labels.assert_not_called() + @pytest.mark.asyncio + async def test_add_label_disabled_by_config(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: + """Test _add_label when label is disabled by configuration.""" + labels_handler.github_webhook.enabled_labels = {"size"} # Only size labels enabled + + # Mock label_exists_in_pull_request to return False (label doesn't exist yet) + with patch.object(labels_handler, "label_exists_in_pull_request", new_callable=AsyncMock) as mock_exists: + mock_exists.return_value = False + + result = await labels_handler._add_label(mock_pull_request, "hold") + + # Verify label was not added (disabled by config) + assert result is False + mock_pull_request.add_to_labels.assert_not_called() + @pytest.mark.asyncio async def test_add_label_static_label(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: """Test _add_label with static label.""" static_label = next(iter(STATIC_LABELS_DICT.keys())) - with patch.object(labels_handler, "label_exists_in_pull_request", new_callable=AsyncMock, return_value=False): - await labels_handler._add_label(mock_pull_request, static_label) - # Verify label was added - mock_pull_request.add_to_labels.assert_called_once_with(static_label) + with patch.object( + labels_handler, "label_exists_in_pull_request", new_callable=AsyncMock, return_value=False + ) as mock_exists: + with patch.object(labels_handler, "wait_for_label", new_callable=AsyncMock, return_value=True): + # Mock repository.get_label to return a mock label + mock_label = Mock() + labels_handler.repository.get_label = Mock(return_value=mock_label) + + await labels_handler._add_label(mock_pull_request, static_label) + + # Verify label_exists_in_pull_request was called + mock_exists.assert_called_once() + # Verify repository.get_label was called to fetch the label + labels_handler.repository.get_label.assert_called_once_with(static_label) + # Verify the label was edited with the correct color + mock_label.edit.assert_called_once() + # Verify add_to_labels was called on the pull request + mock_pull_request.add_to_labels.assert_called_once_with(static_label) @pytest.mark.asyncio async def test_add_label_exception_handling(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: @@ -297,15 +306,23 @@ async def test_add_label_dynamic_label_wait_exception( async def test_add_label_static_label_wait_exception( self, labels_handler: LabelsHandler, mock_pull_request: Mock ) -> None: - """Test _add_label with exception during wait for static label.""" + """Test _add_label with exception during wait for static label. + + When wait_for_label raises an exception, it should propagate (fail-fast). + """ static_label = next(iter(STATIC_LABELS_DICT.keys())) with patch("timeout_sampler.TimeoutWatch") as mock_timeout: mock_timeout.return_value.remaining_time.side_effect = [10, 10, 0] with patch("asyncio.sleep", new_callable=AsyncMock): - with patch.object(labels_handler, "label_exists_in_pull_request", side_effect=[False, True]): - with patch.object(labels_handler, "wait_for_label", side_effect=Exception("Wait failed")): - # Should not raise exception - await labels_handler._add_label(mock_pull_request, static_label) + with patch.object( + labels_handler, "label_exists_in_pull_request", new_callable=AsyncMock, side_effect=[False, True] + ): + with patch.object( + labels_handler, "wait_for_label", new_callable=AsyncMock, side_effect=Exception("Wait failed") + ): + # Exception should propagate (fail-fast architecture) + with pytest.raises(Exception, match="Wait failed"): + await labels_handler._add_label(mock_pull_request, static_label) @pytest.mark.asyncio async def test_wait_for_label_success(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: @@ -435,7 +452,8 @@ async def test_size_label_no_existing_size_label(self, labels_handler: LabelsHan mock_remove.assert_not_called() mock_add.assert_called_once_with(pull_request=pull_request, label=f"{SIZE_LABEL_PREFIX}M") - def test_size_threshold_boundaries(self, labels_handler: LabelsHandler) -> None: + @pytest.mark.asyncio + async def test_size_threshold_boundaries(self, labels_handler: LabelsHandler) -> None: """Test size calculation at threshold boundaries.""" test_cases = [ (19, 0, "XS"), # Just under S threshold (20) @@ -454,7 +472,7 @@ def test_size_threshold_boundaries(self, labels_handler: LabelsHandler) -> None: pull_request = Mock(spec=PullRequest) pull_request.additions = additions pull_request.deletions = deletions - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == f"{SIZE_LABEL_PREFIX}{expected_size}", ( f"Failed for {additions}+{deletions}={additions + deletions}, expected {expected_size}" ) @@ -535,8 +553,8 @@ async def test_manage_reviewed_by_label_changes_requested( ) -> None: """Test manage_reviewed_by_label with changes_requested state.""" with ( - patch.object(labels_handler, "_add_label") as mock_add, - patch.object(labels_handler, "_remove_label") as mock_remove, + patch.object(labels_handler, "_add_label", new_callable=AsyncMock) as mock_add, + patch.object(labels_handler, "_remove_label", new_callable=AsyncMock) as mock_remove, ): await labels_handler.manage_reviewed_by_label(mock_pull_request, "changes_requested", ADD_STR, "reviewer1") mock_add.assert_called_once() @@ -570,8 +588,8 @@ async def test_label_by_user_comment_remove(self, labels_handler: LabelsHandler, @pytest.mark.asyncio async def test_add_size_label_no_size_label(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: """Test add_size_label when get_size returns None.""" - with patch.object(labels_handler, "get_size", return_value=None): - with patch.object(labels_handler, "_add_label") as mock_add: + with patch.object(labels_handler, "get_size", new_callable=AsyncMock, return_value=None): + with patch.object(labels_handler, "_add_label", new_callable=AsyncMock) as mock_add: await labels_handler.add_size_label(mock_pull_request) mock_add.assert_not_called() @@ -594,7 +612,9 @@ async def test_add_size_label_remove_existing_exception( mock_pull_request.additions = 10 mock_pull_request.deletions = 5 existing_size_label = f"{SIZE_LABEL_PREFIX}L" - with patch.object(labels_handler, "pull_request_labels_names", return_value=[existing_size_label]): + with patch.object( + labels_handler, "pull_request_labels_names", new_callable=AsyncMock, return_value=[existing_size_label] + ): with patch.object( labels_handler, "_remove_label", new_callable=AsyncMock, side_effect=Exception("Remove failed") ): @@ -629,7 +649,7 @@ async def test_label_by_user_comment_approve_add( self, labels_handler: LabelsHandler, mock_pull_request: Mock ) -> None: """Test label_by_user_comment for approve addition.""" - with patch.object(labels_handler, "manage_reviewed_by_label") as mock_manage: + with patch.object(labels_handler, "manage_reviewed_by_label", new_callable=AsyncMock) as mock_manage: await labels_handler.label_by_user_comment( pull_request=mock_pull_request, user_requested_label=APPROVE_STR, @@ -882,7 +902,8 @@ def test_get_custom_pr_size_thresholds_invalid_color(self, mock_github_webhook: ] assert thresholds == expected - def test_get_size_with_custom_thresholds(self, mock_github_webhook: Mock) -> None: + @pytest.mark.asyncio + async def test_get_size_with_custom_thresholds(self, mock_github_webhook: Mock) -> None: """Test get_size using custom thresholds.""" # Mock config with custom thresholds mock_github_webhook.config.get_value.return_value = { @@ -906,10 +927,11 @@ def test_get_size_with_custom_thresholds(self, mock_github_webhook: Mock) -> Non pull_request.additions = additions pull_request.deletions = deletions - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == expected - def test_get_size_with_single_custom_threshold(self, mock_github_webhook: Mock) -> None: + @pytest.mark.asyncio + async def test_get_size_with_single_custom_threshold(self, mock_github_webhook: Mock) -> None: """Test get_size with only one custom threshold.""" # Mock config with single threshold mock_github_webhook.config.get_value.return_value = { @@ -929,7 +951,7 @@ def test_get_size_with_single_custom_threshold(self, mock_github_webhook: Mock) pull_request.additions = additions pull_request.deletions = deletions - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == expected def test_custom_threshold_sorting(self, mock_github_webhook: Mock) -> None: @@ -1085,7 +1107,8 @@ def test_get_custom_pr_size_thresholds_mixed_with_infinity(self, mock_github_web for i in range(len(thresholds) - 1): assert thresholds[i][0] < thresholds[i + 1][0] - def test_get_size_with_infinity_threshold(self, mock_github_webhook: Mock) -> None: + @pytest.mark.asyncio + async def test_get_size_with_infinity_threshold(self, mock_github_webhook: Mock) -> None: """Test get_size() method with custom infinity threshold.""" # Mock config with infinity threshold mock_github_webhook.config.get_value.return_value = { @@ -1110,7 +1133,7 @@ def test_get_size_with_infinity_threshold(self, mock_github_webhook: Mock) -> No pull_request.additions = additions pull_request.deletions = deletions - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == expected, ( f"Failed for {additions}+{deletions}={additions + deletions}, expected {expected}" ) @@ -1130,3 +1153,176 @@ def test_get_label_color_with_infinity_threshold(self, mock_github_webhook: Mock assert labels_handler._get_label_color("size/S") == "008000" # green hex assert labels_handler._get_label_color("size/M") == "ffa500" # orange hex assert labels_handler._get_label_color("size/XXL") == "ff0000" # red hex (infinity category) + + +class TestIsLabelEnabled: + """Tests for is_label_enabled method.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Mock GitHub webhook handler.""" + webhook = Mock() + webhook.repository = Mock() + webhook.log_prefix = "[TEST]" + webhook.logger = Mock() + webhook.config.get_value.return_value = None + webhook.ctx = None + webhook.enabled_labels = None + webhook.label_colors = {} + return webhook + + @pytest.fixture + def labels_handler(self, mock_github_webhook: Mock) -> LabelsHandler: + """Labels handler instance.""" + return LabelsHandler(github_webhook=mock_github_webhook, owners_file_handler=Mock()) + + def test_all_labels_enabled_when_not_configured(self, labels_handler: LabelsHandler) -> None: + """When enabled_labels is None, all labels should be enabled.""" + labels_handler.github_webhook.enabled_labels = None + + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is True + assert labels_handler.is_label_enabled(HOLD_LABEL_STR) is True + assert labels_handler.is_label_enabled(WIP_STR) is True + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}M") is True + assert labels_handler.is_label_enabled(f"{BRANCH_LABEL_PREFIX}main") is True + + def test_reviewed_by_labels_always_enabled(self, labels_handler: LabelsHandler) -> None: + """reviewed-by labels should always be enabled, even if not in config.""" + labels_handler.github_webhook.enabled_labels = set() # Empty set - nothing enabled + + # reviewed-by labels should still be enabled + assert labels_handler.is_label_enabled("approved-user1") is True + assert labels_handler.is_label_enabled("lgtm-user2") is True + assert labels_handler.is_label_enabled("changes-requested-user3") is True + assert labels_handler.is_label_enabled("commented-user4") is True + + def test_specific_labels_enabled(self, labels_handler: LabelsHandler) -> None: + """Only labels in enabled_labels should be enabled.""" + labels_handler.github_webhook.enabled_labels = {"verified", "hold", "size"} + + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is True + assert labels_handler.is_label_enabled(HOLD_LABEL_STR) is True + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}M") is True + assert labels_handler.is_label_enabled(WIP_STR) is False + assert labels_handler.is_label_enabled("needs-rebase") is False + assert labels_handler.is_label_enabled(f"{BRANCH_LABEL_PREFIX}main") is False + + def test_branch_labels_category(self, labels_handler: LabelsHandler) -> None: + """Branch labels should be controlled by 'branch' category.""" + labels_handler.github_webhook.enabled_labels = {"branch"} + + assert labels_handler.is_label_enabled(f"{BRANCH_LABEL_PREFIX}main") is True + assert labels_handler.is_label_enabled(f"{BRANCH_LABEL_PREFIX}feature-123") is True + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is False + + def test_cherry_pick_labels_category(self, labels_handler: LabelsHandler) -> None: + """Cherry-pick labels should be controlled by 'cherry-pick' category.""" + labels_handler.github_webhook.enabled_labels = {"cherry-pick"} + + assert labels_handler.is_label_enabled(f"{CHERRY_PICK_LABEL_PREFIX}v1.0") is True + assert labels_handler.is_label_enabled(CHERRY_PICKED_LABEL) is True + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is False + + def test_unknown_labels_allowed_by_default(self, labels_handler: LabelsHandler) -> None: + """Unknown labels should be allowed when enabled_labels is set.""" + labels_handler.github_webhook.enabled_labels = {"verified"} + + # Unknown label should be allowed + assert labels_handler.is_label_enabled("custom-label") is True + + def test_empty_enabled_labels_disables_all_except_reviewed_by(self, labels_handler: LabelsHandler) -> None: + """Empty enabled_labels should disable all configurable labels.""" + labels_handler.github_webhook.enabled_labels = set() + + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is False + assert labels_handler.is_label_enabled(HOLD_LABEL_STR) is False + assert labels_handler.is_label_enabled(WIP_STR) is False + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}M") is False + assert labels_handler.is_label_enabled(f"{BRANCH_LABEL_PREFIX}main") is False + # reviewed-by always enabled + assert labels_handler.is_label_enabled("approved-user1") is True + assert labels_handler.is_label_enabled("lgtm-user2") is True + + def test_lgtm_and_approve_always_enabled(self, labels_handler: LabelsHandler) -> None: + """The exact 'lgtm' and 'approve' labels should always be enabled. + + These are required for the review workflow and cannot be disabled, + even when enabled_labels is set to an empty set or doesn't include + their categories. + """ + labels_handler.github_webhook.enabled_labels = set() # Empty - nothing enabled + + # Exact lgtm and approve labels should always be enabled + assert labels_handler.is_label_enabled(LGTM_STR) is True + assert labels_handler.is_label_enabled(APPROVE_STR) is True + + # Also verify with a restrictive enabled_labels that doesn't include them + labels_handler.github_webhook.enabled_labels = {"hold", "verified"} + assert labels_handler.is_label_enabled(LGTM_STR) is True + assert labels_handler.is_label_enabled(APPROVE_STR) is True + + def test_automerge_label_category(self, labels_handler: LabelsHandler) -> None: + """Automerge label should be controlled by 'automerge' category.""" + labels_handler.github_webhook.enabled_labels = {AUTOMERGE_LABEL_STR} + + assert labels_handler.is_label_enabled(AUTOMERGE_LABEL_STR) is True + assert labels_handler.is_label_enabled(VERIFIED_LABEL_STR) is False + + def test_size_labels_with_custom_names(self, labels_handler: LabelsHandler) -> None: + """Size labels with custom names should still be controlled by 'size' category.""" + labels_handler.github_webhook.enabled_labels = {"size"} + + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}XS") is True + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}CustomName") is True + assert labels_handler.is_label_enabled(f"{SIZE_LABEL_PREFIX}Massive") is True + + +class TestCustomLabelColors: + """Tests for custom label colors configuration.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Mock GitHub webhook handler.""" + webhook = Mock() + webhook.repository = Mock() + webhook.log_prefix = "[TEST]" + webhook.logger = Mock() + webhook.config.get_value.return_value = None + webhook.ctx = None + webhook.enabled_labels = None + webhook.label_colors = {} + return webhook + + @pytest.fixture + def labels_handler(self, mock_github_webhook: Mock) -> LabelsHandler: + """Labels handler instance.""" + return LabelsHandler(github_webhook=mock_github_webhook, owners_file_handler=Mock()) + + def test_custom_color_for_static_label(self, labels_handler: LabelsHandler) -> None: + """Custom colors should override defaults for static labels.""" + labels_handler.github_webhook.label_colors = {"hold": "blue"} + + color = labels_handler._get_label_color("hold") + assert color == "0000ff" # blue in hex + + def test_custom_color_for_dynamic_label_prefix(self, labels_handler: LabelsHandler) -> None: + """Custom colors should work for dynamic label prefixes.""" + labels_handler.github_webhook.label_colors = {"approved-": "purple"} + + color = labels_handler._get_label_color("approved-username") + assert color == "800080" # purple in hex + + def test_fallback_to_default_color(self, labels_handler: LabelsHandler) -> None: + """When no custom color, should use default color.""" + labels_handler.github_webhook.label_colors = {} + + color = labels_handler._get_label_color("hold") + assert color == "B60205" # Default hold color + + def test_invalid_color_name_fallback(self, labels_handler: LabelsHandler) -> None: + """Invalid color names should fallback to default.""" + labels_handler.github_webhook.label_colors = {"hold": "notacolor"} + + color = labels_handler._get_label_color("hold") + # Should use lightgray fallback + assert color == "d3d3d3" diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 78a159294..5590610dc 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -14,9 +14,10 @@ CAN_BE_MERGED_STR, CHANGED_REQUESTED_BY_LABEL_PREFIX, CHERRY_PICK_LABEL_PREFIX, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, COMMENTED_BY_LABEL_PREFIX, HAS_CONFLICTS_LABEL_STR, + HOLD_LABEL_STR, LGTM_BY_LABEL_PREFIX, NEEDS_REBASE_LABEL_STR, TOX_STR, @@ -81,6 +82,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.auto_verify_cherry_picked_prs = True mock_webhook.last_commit = Mock() mock_webhook.ctx = None + mock_webhook.enabled_labels = None # Default: all labels enabled return mock_webhook @pytest.fixture @@ -402,6 +404,45 @@ def test_prepare_retest_welcome_comment(self, pull_request_handler: PullRequestH assert TOX_STR in result assert "pre-commit" in result + def test_prepare_no_blockers_requirement_all_enabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test no blockers requirement when all labels are enabled.""" + # Default: enabled_labels is None, so all are enabled + # Mock is_label_enabled to return True for all labels + pull_request_handler.labels_handler.is_label_enabled = Mock(return_value=True) + result = pull_request_handler._prepare_no_blockers_requirement + assert "No WIP, hold, conflict labels" in result + + def test_prepare_no_blockers_requirement_wip_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test no blockers requirement when wip is disabled.""" + # Mock is_label_enabled: wip disabled, hold enabled + pull_request_handler.labels_handler.is_label_enabled = Mock(side_effect=lambda label: label != WIP_STR) + result = pull_request_handler._prepare_no_blockers_requirement + assert "WIP" not in result + assert "hold" in result + assert "conflict" in result + + def test_prepare_no_blockers_requirement_hold_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test no blockers requirement when hold is disabled.""" + # Mock is_label_enabled: hold disabled, wip enabled + pull_request_handler.labels_handler.is_label_enabled = Mock(side_effect=lambda label: label != HOLD_LABEL_STR) + result = pull_request_handler._prepare_no_blockers_requirement + assert "WIP" in result + assert "hold" not in result + assert "conflict" in result + + def test_prepare_no_blockers_requirement_both_disabled(self, pull_request_handler: PullRequestHandler) -> None: + """Test no blockers requirement when both wip and hold are disabled.""" + # Mock is_label_enabled: both wip and hold disabled + pull_request_handler.labels_handler.is_label_enabled = Mock( + side_effect=lambda label: label not in (WIP_STR, HOLD_LABEL_STR) + ) + result = pull_request_handler._prepare_no_blockers_requirement + assert "WIP" not in result + assert "hold" not in result + assert "conflict" in result + # Only conflict should be present + assert "No conflict labels" in result + @pytest.mark.asyncio async def test_label_all_opened_pull_requests_merge_state_after_merged( self, pull_request_handler: PullRequestHandler @@ -650,7 +691,7 @@ async def test_process_verified_cherry_picked_pr_auto_verify_enabled( mock_pull_request = Mock(spec=PullRequest) mock_label = Mock() - mock_label.name = CHERRY_PICKED_LABEL_PREFIX + mock_label.name = CHERRY_PICKED_LABEL mock_pull_request.labels = [mock_label] with ( @@ -671,7 +712,7 @@ async def test_process_verified_cherry_picked_pr_auto_verify_disabled( mock_pull_request = Mock(spec=PullRequest) mock_label = Mock() - mock_label.name = CHERRY_PICKED_LABEL_PREFIX + mock_label.name = CHERRY_PICKED_LABEL mock_pull_request.labels = [mock_label] with ( @@ -2000,3 +2041,95 @@ async def test_label_pull_request_by_merge_state_incomplete_compare_data( pull_request_handler.labels_handler._add_label.assert_called_once_with( pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR ) + + @pytest.mark.asyncio + async def test_regenerate_welcome_message_existing_comment_updated( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test regenerating welcome message when existing welcome comment is found and updated.""" + # Create a mock existing comment containing the welcome message URL + mock_comment = Mock() + mock_comment.body = "Some text welcome-message-url more text" + mock_comment.edit = Mock() + mock_pull_request.get_issue_comments = Mock(return_value=[mock_comment]) + + with patch.object(pull_request_handler, "_prepare_welcome_comment", return_value="New welcome message"): + await pull_request_handler.regenerate_welcome_message(mock_pull_request) + + # Verify comment.edit was called with new welcome message + mock_comment.edit.assert_called_once_with(body="New welcome message") + # Verify create_issue_comment was NOT called since existing comment was found + mock_pull_request.create_issue_comment.assert_not_called() + # Verify logging + pull_request_handler.logger.info.assert_called_with("[TEST] Updated existing welcome message") + + @pytest.mark.asyncio + async def test_regenerate_welcome_message_no_existing_comment_creates_new( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test regenerating welcome message when no existing welcome comment is found.""" + # Empty comment list - no welcome message exists + mock_pull_request.get_issue_comments = Mock(return_value=[]) + + with patch.object(pull_request_handler, "_prepare_welcome_comment", return_value="New welcome message"): + await pull_request_handler.regenerate_welcome_message(mock_pull_request) + + # Verify create_issue_comment was called with new welcome message + mock_pull_request.create_issue_comment.assert_called_once_with(body="New welcome message") + # Verify logging + pull_request_handler.logger.info.assert_called_with("[TEST] Creating new welcome message") + + @pytest.mark.asyncio + async def test_regenerate_welcome_message_other_comments_not_matched( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test regenerating welcome message ignores comments without welcome URL marker.""" + # Create comments that don't contain the welcome message URL + mock_comment1 = Mock() + mock_comment1.body = "Some unrelated comment" + mock_comment1.edit = Mock() + + mock_comment2 = Mock() + mock_comment2.body = "Another unrelated comment" + mock_comment2.edit = Mock() + + mock_pull_request.get_issue_comments = Mock(return_value=[mock_comment1, mock_comment2]) + + with patch.object(pull_request_handler, "_prepare_welcome_comment", return_value="New welcome message"): + await pull_request_handler.regenerate_welcome_message(mock_pull_request) + + # Verify neither comment was edited + mock_comment1.edit.assert_not_called() + mock_comment2.edit.assert_not_called() + # Verify new comment was created + mock_pull_request.create_issue_comment.assert_called_once_with(body="New welcome message") + + @pytest.mark.asyncio + async def test_regenerate_welcome_message_finds_correct_comment_among_many( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that regenerate finds the correct welcome comment among multiple comments.""" + # Create multiple comments, only one with the welcome URL + mock_comment1 = Mock() + mock_comment1.body = "Some unrelated comment" + mock_comment1.edit = Mock() + + mock_welcome_comment = Mock() + mock_welcome_comment.body = "welcome-message-url\n## Welcome!" + mock_welcome_comment.edit = Mock() + + mock_comment3 = Mock() + mock_comment3.body = "Another comment" + mock_comment3.edit = Mock() + + mock_pull_request.get_issue_comments = Mock(return_value=[mock_comment1, mock_welcome_comment, mock_comment3]) + + with patch.object(pull_request_handler, "_prepare_welcome_comment", return_value="Updated welcome"): + await pull_request_handler.regenerate_welcome_message(mock_pull_request) + + # Verify only the welcome comment was edited + mock_comment1.edit.assert_not_called() + mock_welcome_comment.edit.assert_called_once_with(body="Updated welcome") + mock_comment3.edit.assert_not_called() + # Verify no new comment was created + mock_pull_request.create_issue_comment.assert_not_called() diff --git a/webhook_server/tests/test_pull_request_size.py b/webhook_server/tests/test_pull_request_size.py index 5fbfd7737..f24de23fd 100644 --- a/webhook_server/tests/test_pull_request_size.py +++ b/webhook_server/tests/test_pull_request_size.py @@ -5,6 +5,7 @@ from webhook_server.utils.constants import SIZE_LABEL_PREFIX +@pytest.mark.asyncio @pytest.mark.parametrize( "additions, deletions, expected_label", [ @@ -17,9 +18,9 @@ (1000, 1, "XXL"), ], ) -def test_get_size_thresholds(process_github_webhook, owners_file_handler, additions, deletions, expected_label): +async def test_get_size_thresholds(process_github_webhook, owners_file_handler, additions, deletions, expected_label): pull_request = PullRequest(additions=additions, deletions=deletions) labels_handler = LabelsHandler(github_webhook=process_github_webhook, owners_file_handler=owners_file_handler) - result = labels_handler.get_size(pull_request=pull_request) + result = await labels_handler.get_size(pull_request=pull_request) assert result == f"{SIZE_LABEL_PREFIX}{expected_label}" diff --git a/webhook_server/tests/test_safe_rotating_handler.py b/webhook_server/tests/test_safe_rotating_handler.py new file mode 100644 index 000000000..1bfb924c1 --- /dev/null +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -0,0 +1,233 @@ +"""Tests for SafeRotatingFileHandler.""" + +from __future__ import annotations + +import logging +import os +import tempfile +from unittest.mock import patch + +import simple_logger.logger + +import webhook_server.utils.helpers # noqa: F401 +from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler + + +class TestSafeRotatingFileHandler: + """Tests for SafeRotatingFileHandler crash resilience.""" + + def test_basic_rollover_works(self) -> None: + """Test that basic rollover works normally.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=3, + ) + handler.setFormatter(logging.Formatter("%(message)s")) + try: + # Write enough data to trigger rollover + for _ in range(20): + record = logging.LogRecord( + name="test", + level=logging.INFO, + pathname="", + lineno=0, + msg="X" * 50, + args=(), + exc_info=None, + ) + handler.emit(record) + finally: + handler.close() + + def test_rollover_handles_missing_backup_files(self) -> None: + """Test that rollover gracefully handles missing backup files.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=5, + ) + try: + # Create the log file + with open(log_file, "w") as f: + f.write("initial content") + + # Manually trigger rollover - this should not crash + # even if backup files don't exist + handler.doRollover() + + # Verify log file can still be used + assert handler.stream is not None + + finally: + handler.close() + + def test_rollover_handles_file_deleted_during_operation(self) -> None: + """Test rollover handles files deleted between exists() and operation.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=3, + ) + try: + # Create the log file + with open(log_file, "w") as f: + f.write("initial content") + + # Mock os.exists to return True, but os.remove will raise FileNotFoundError + original_exists = os.path.exists + + def mock_exists(path: str) -> bool: + if path.endswith(".1"): + return True # Pretend .1 exists + return original_exists(path) + + original_remove = os.remove + + def mock_remove(path: str) -> None: + if path.endswith(".1"): + raise FileNotFoundError() + return original_remove(path) + + with patch("os.path.exists", side_effect=mock_exists): + with patch("os.remove", side_effect=mock_remove): + # This should not crash + handler.doRollover() + + # Verify handler is still functional + assert handler.stream is not None + + finally: + handler.close() + + def test_rollover_handles_rename_file_not_found(self) -> None: + """Test rollover handles FileNotFoundError during rename.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=3, + ) + try: + # Create the log file + with open(log_file, "w") as f: + f.write("initial content") + + original_exists = os.path.exists + + def mock_exists(path: str) -> bool: + if ".1" in path or ".2" in path: + return True # Pretend backup files exist + return original_exists(path) + + original_rename = os.rename + + def mock_rename(src: str, dst: str) -> None: + if ".1" in src or ".2" in src: + raise FileNotFoundError() + return original_rename(src, dst) + + with patch("os.path.exists", side_effect=mock_exists): + with patch("os.rename", side_effect=mock_rename): + # This should not crash + handler.doRollover() + + # Verify handler is still functional + assert handler.stream is not None + + finally: + handler.close() + + def test_rollover_with_nonexistent_base_file(self) -> None: + """Test rollover when base file is deleted before rotate.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=3, + ) + try: + # Create the log file + with open(log_file, "w") as f: + f.write("initial content") + + # Open handler's stream + if handler.stream is None: + handler.stream = handler._open() + + # Patch os.path.exists to return False for the base log file, + # simulating the scenario where the file was deleted before rollover. + # This avoids actually removing the file while handler.stream is open, + # which would fail on Windows. + original_exists = os.path.exists + + def mock_exists(path: str) -> bool: + if path == log_file: + return False # Simulate base file not existing + return original_exists(path) + + with patch("os.path.exists", side_effect=mock_exists): + # This should not crash + handler.doRollover() + + # Verify handler created a new file + assert handler.stream is not None + + finally: + handler.close() + + def test_rollover_handles_open_failure(self) -> None: + """Test rollover handles OSError when opening new log file.""" + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "test.log") + handler = SafeRotatingFileHandler( + filename=log_file, + maxBytes=100, + backupCount=3, + ) + try: + # Create the log file and open the stream + with open(log_file, "w") as f: + f.write("initial content") + handler.stream = handler._open() + + # Mock _open to raise OSError (e.g., permission denied, disk full) + def mock_open() -> None: + raise OSError("Permission denied") + + with patch.object(handler, "_open", side_effect=mock_open): + # This should not crash - stream will be left as None + handler.doRollover() + + # Stream should be None since _open failed + assert handler.stream is None + + # Verify handler is still usable - emit() will try to reopen + # Restore normal _open behavior for this check + handler.stream = handler._open() + assert handler.stream is not None + + finally: + handler.close() + + +class TestSafeRotatingHandlerPatch: + """Test that simple_logger is patched correctly.""" + + def test_simple_logger_uses_safe_handler(self) -> None: + """Test that importing helpers patches simple_logger. + + The helpers module patches simple_logger.logger.RotatingFileHandler + at import time. Since helpers is imported transitively by many modules + in this project, the patch should already be in place. + """ + # Verify the patch is in place (helpers is imported at module level above) + assert simple_logger.logger.RotatingFileHandler is SafeRotatingFileHandler diff --git a/webhook_server/utils/app_utils.py b/webhook_server/utils/app_utils.py index dff57f25a..ac67c2771 100644 --- a/webhook_server/utils/app_utils.py +++ b/webhook_server/utils/app_utils.py @@ -5,6 +5,7 @@ import hmac import ipaddress import logging +from ipaddress import IPv4Network, IPv6Network from typing import Any import httpx @@ -77,7 +78,7 @@ def verify_signature(payload_body: bytes, secret_token: str, signature_header: s raise HTTPException(status_code=403, detail="Request signatures didn't match!") -async def gate_by_allowlist_ips(request: Request, allowed_ips: tuple[ipaddress._BaseNetwork, ...]) -> None: +async def gate_by_allowlist_ips(request: Request, allowed_ips: tuple[IPv4Network | IPv6Network, ...]) -> None: """Gate access by IP allowlist.""" if allowed_ips: if not request.client or not request.client.host: @@ -176,21 +177,18 @@ def log_webhook_summary(ctx: WebhookContext, logger: logging.Logger, log_prefix: raise ValueError("Context completed_at is None - context not completed") duration_ms = int((ctx.completed_at - ctx.started_at).total_seconds() * 1000) - # Build summary of workflow steps - validate required fields + # Build summary of workflow steps - handle incomplete steps gracefully steps_summary = [] for step_name, step_data in ctx.workflow_steps.items(): - if "status" not in step_data: - raise ValueError( - f"Workflow step '{step_name}' missing 'status' field - ensure complete_step() or fail_step() was called" - ) - if "duration_ms" not in step_data or step_data["duration_ms"] is None: - raise ValueError( - f"Workflow step '{step_name}' missing or None 'duration_ms' field - " - "ensure complete_step() or fail_step() was called" - ) - status = step_data["status"] - step_duration_ms = step_data["duration_ms"] - steps_summary.append(f"{step_name}:{status}({format_duration(step_duration_ms)})") + status = step_data.get("status", "unknown") + step_duration_ms = step_data.get("duration_ms") + + # Handle incomplete steps (started but not completed/failed due to exception) + if step_duration_ms is None: + # Step was started but never completed - mark as incomplete + steps_summary.append(f"{step_name}:{status}(incomplete)") + else: + steps_summary.append(f"{step_name}:{status}({format_duration(step_duration_ms)})") steps_str = ", ".join(steps_summary) if steps_summary else "no steps recorded" diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 17ca4a00d..f40c77f0a 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -1,3 +1,6 @@ +import types +from collections.abc import Mapping + OTHER_MAIN_BRANCH: str = "master" TOX_STR: str = "tox" PRE_COMMIT_STR: str = "pre-commit" @@ -18,7 +21,7 @@ APPROVE_STR: str = "approve" LABELS_SEPARATOR: str = "-" CHERRY_PICK_LABEL_PREFIX: str = f"cherry-pick{LABELS_SEPARATOR}" -CHERRY_PICKED_LABEL_PREFIX: str = "CherryPicked" +CHERRY_PICKED_LABEL: str = "CherryPicked" APPROVED_BY_LABEL_PREFIX: str = f"approved{LABELS_SEPARATOR}" LGTM_BY_LABEL_PREFIX: str = f"{LGTM_STR}{LABELS_SEPARATOR}" CHANGED_REQUESTED_BY_LABEL_PREFIX: str = f"changes-requested{LABELS_SEPARATOR}" @@ -37,6 +40,7 @@ COMMAND_ASSIGN_REVIEWER_STR: str = "assign-reviewer" COMMAND_ADD_ALLOWED_USER_STR: str = "add-allowed-user" COMMAND_AUTOMERGE_STR: str = "automerge" +COMMAND_REGENERATE_WELCOME_STR: str = "regenerate-welcome" AUTOMERGE_LABEL_STR: str = "automerge" ROOT_APPROVERS_KEY: str = "root-approvers" @@ -50,9 +54,34 @@ AUTOMERGE_LABEL_STR: "0E8A16", } +# Mapping from exact label strings to their configuration category names. +# +# IMPORTANT: This map is for EXACT-MATCH labels only! +# - Keys must be complete label names (e.g., "hold", "verified", "wip") +# - These are looked up directly: `if label in LABEL_CATEGORY_MAP` +# +# PREFIX-BASED labels are NOT in this map and are handled separately +# by prefix-matching logic in LabelsHandler.is_label_enabled(): +# - "size" category: labels like "size/XS", "size/M", "size/XXL" (prefix: SIZE_LABEL_PREFIX) +# - "branch" category: labels like "branch-main", "branch-feature" (prefix: BRANCH_LABEL_PREFIX) +# - "cherry-pick" category: labels like "cherry-pick-main" (prefix: CHERRY_PICK_LABEL_PREFIX) +# +# Do NOT add prefix-based label examples (e.g., "size/XL", "branch-main") to this map. +LABEL_CATEGORY_MAP: dict[str, str] = { + HOLD_LABEL_STR: "hold", + VERIFIED_LABEL_STR: "verified", + WIP_STR: "wip", + AUTOMERGE_LABEL_STR: "automerge", + LGTM_STR: "lgtm", # Always enabled + APPROVE_STR: "approve", # Always enabled + NEEDS_REBASE_LABEL_STR: "needs-rebase", + HAS_CONFLICTS_LABEL_STR: "has-conflicts", + CAN_BE_MERGED_STR: "can-be-merged", +} + STATIC_LABELS_DICT: dict[str, str] = { **USER_LABELS_DICT, - CHERRY_PICKED_LABEL_PREFIX: "1D76DB", + CHERRY_PICKED_LABEL: "1D76DB", f"{SIZE_LABEL_PREFIX}L": "F5621C", f"{SIZE_LABEL_PREFIX}M": "F09C74", f"{SIZE_LABEL_PREFIX}S": "0E8A16", @@ -73,7 +102,28 @@ BRANCH_LABEL_PREFIX: "1D76DB", } -ALL_LABELS_DICT: dict[str, str] = {**STATIC_LABELS_DICT, **DYNAMIC_LABELS_DICT} +_ALL_LABELS_DICT: dict[str, str] = {**STATIC_LABELS_DICT, **DYNAMIC_LABELS_DICT} +ALL_LABELS_DICT: Mapping[str, str] = types.MappingProxyType(_ALL_LABELS_DICT) + +# Default label colors - uses ALL_LABELS_DICT as the source of truth +# These are used when no custom colors are configured via labels.colors +# Using MappingProxyType to prevent accidental mutation of the shared dict +DEFAULT_LABEL_COLORS: Mapping[str, str] = ALL_LABELS_DICT + +# All configurable label categories (for enabled-labels config) +# Note: reviewed-by is NOT in this list because it cannot be disabled +CONFIGURABLE_LABEL_CATEGORIES: set[str] = { + "verified", + "hold", + "wip", + "needs-rebase", + "has-conflicts", + "can-be-merged", + "size", + "branch", + "cherry-pick", + "automerge", +} class REACTIONS: diff --git a/webhook_server/utils/github_repository_and_webhook_settings.py b/webhook_server/utils/github_repository_and_webhook_settings.py index d5caa3f85..7664bd0b7 100644 --- a/webhook_server/utils/github_repository_and_webhook_settings.py +++ b/webhook_server/utils/github_repository_and_webhook_settings.py @@ -1,4 +1,4 @@ -from concurrent.futures import ThreadPoolExecutor, as_completed +from concurrent.futures import Future, ThreadPoolExecutor, as_completed from typing import Any import github @@ -24,7 +24,7 @@ async def repository_and_webhook_settings(webhook_secret: str | None = None) -> config = Config(logger=LOGGER) apis_dict: dict[str, dict[str, Any]] = {} - apis: list = [] + apis: list[Future[tuple[str, github.Github | None, str]]] = [] with ThreadPoolExecutor() as executor: for repo, _ in config.root_data["repositories"].items(): apis.append( @@ -34,9 +34,9 @@ async def repository_and_webhook_settings(webhook_secret: str | None = None) -> ) ) - for result in as_completed(apis): - repository, github_api, api_user = result.result() - apis_dict[repository] = {"api": github_api, "user": api_user} + for result in as_completed(apis): + repository, github_api, api_user = result.result() + apis_dict[repository] = {"api": github_api, "user": api_user} LOGGER.debug(f"Repositories APIs: {apis_dict}") diff --git a/webhook_server/utils/github_repository_settings.py b/webhook_server/utils/github_repository_settings.py index b7cbec793..087492e8d 100644 --- a/webhook_server/utils/github_repository_settings.py +++ b/webhook_server/utils/github_repository_settings.py @@ -251,7 +251,7 @@ def set_repository( apis_dict: dict[str, dict[str, Any]], branch_protection: dict[str, Any], config: Config, -) -> tuple[bool, str, Callable]: +) -> tuple[bool, str, Callable[..., Any]]: full_repository_name: str = data["name"] LOGGER.info(f"Processing repository {full_repository_name}") protected_branches: dict[str, Any] = config.get_value(value="protected-branches", return_on_none={}) @@ -276,7 +276,7 @@ def set_repository( LOGGER.warning, ) - futures: list[Future] = [] + futures: list[Future[Any]] = [] with ThreadPoolExecutor() as executor: for branch_name, status_checks in protected_branches.items(): @@ -339,7 +339,7 @@ def set_all_in_progress_check_runs_to_queued(repo_config: Config, apis_dict: dic BUILD_CONTAINER_STR, PRE_COMMIT_STR, ) - futures: list[Future] = [] + futures: list[Future[Any]] = [] with ThreadPoolExecutor() as executor: for repo, data in repo_config.root_data["repositories"].items(): @@ -366,7 +366,7 @@ def set_repository_check_runs_to_queued( github_api: Github, check_runs: tuple[str], api_user: str, -) -> tuple[bool, str, Callable]: +) -> tuple[bool, str, Callable[..., Any]]: def _set_checkrun_queued(_api: Repository, _pull_request: PullRequest) -> None: # Avoid materializing all commits - use single-pass iteration to find last commit # This is O(1) memory instead of O(N) for large PRs diff --git a/webhook_server/utils/helpers.py b/webhook_server/utils/helpers.py index 16d9301e8..5a0521cfe 100644 --- a/webhook_server/utils/helpers.py +++ b/webhook_server/utils/helpers.py @@ -17,6 +17,7 @@ from uuid import uuid4 import github +import simple_logger.logger from colorama import Fore from github import GithubException from github.RateLimitOverview import RateLimitOverview @@ -26,6 +27,11 @@ from webhook_server.libs.config import Config from webhook_server.libs.exceptions import NoApiTokenError +from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler + +# Patch simple_logger to use SafeRotatingFileHandler to prevent crashes +# when backup log files are missing during rollover +simple_logger.logger.RotatingFileHandler = SafeRotatingFileHandler def get_logger_with_params( @@ -511,7 +517,7 @@ def log_rate_limit(rate_limit: RateLimitOverview, api_user: str) -> None: logger.warning(msg) -def get_future_results(futures: list[Future]) -> None: +def get_future_results(futures: list[Future[Any]]) -> None: """ Process futures from repository configuration tasks. diff --git a/webhook_server/utils/safe_rotating_handler.py b/webhook_server/utils/safe_rotating_handler.py new file mode 100644 index 000000000..983e9d7e8 --- /dev/null +++ b/webhook_server/utils/safe_rotating_handler.py @@ -0,0 +1,111 @@ +"""Safe rotating file handler that handles missing backup files gracefully. + +Design Rationale - Broad OSError Suppression: + This handler intentionally suppresses all OSError exceptions during rollover + operations. This is a deliberate design choice based on the principle that + **logging must NEVER crash the application**, even when file operations fail. + + Why broad suppression instead of specific exceptions: + 1. Race conditions: Files can be deleted between exists() check and operation + (FileNotFoundError, but also PermissionError if replaced by protected file) + 2. Disk full: ENOSPC can occur mid-operation, but logging should continue + 3. Permission changes: Files may become unwritable during rotation + 4. Network filesystems: Various transient errors can occur on NFS/CIFS + 5. Container environments: Filesystem can change unexpectedly + + The alternative (letting errors propagate) would cause: + - Application crashes from logging failures + - Lost log entries for the actual application errors + - Cascading failures in webhook processing + + Trade-off accepted: Some rotation failures may go unnoticed. This is acceptable + because the primary log file will be recreated and logging will continue. + The handler prioritizes availability over perfect rotation. +""" + +from __future__ import annotations + +import os +from logging.handlers import RotatingFileHandler + + +class SafeRotatingFileHandler(RotatingFileHandler): + """RotatingFileHandler that handles missing backup files during rollover. + + The standard RotatingFileHandler can crash with FileNotFoundError when + backup log files are deleted externally (e.g., by logrotate, manual cleanup, + or disk space management) during the rollover process. + + This implementation catches all OSError exceptions during doRollover and + continues gracefully. This is intentional - logging infrastructure must + never crash the application, even if rotation fails. See module docstring + for detailed rationale. + """ + + def doRollover(self) -> None: + """Perform log file rollover with graceful handling of file errors. + + Suppresses all OSError exceptions during rollover to ensure logging + never crashes the application. This includes: + - FileNotFoundError: Files deleted between check and operation + - PermissionError: Files became unwritable + - OSError: Disk full, network filesystem errors, etc. + + See module docstring for detailed rationale on this design choice. + + Note: + This method does not log warnings internally to avoid recursion. + Callers should perform external monitoring if explicit notification + of rotation failures is required. + """ + if self.stream: + self.stream.close() + self.stream = None # type: ignore[assignment] + + if self.backupCount > 0: + # Remove backup files that exceed backupCount, handle missing files + for i in range(self.backupCount - 1, 0, -1): + sfn = self.rotation_filename(f"{self.baseFilename}.{i}") + dfn = self.rotation_filename(f"{self.baseFilename}.{i + 1}") + if os.path.exists(sfn): + try: + if os.path.exists(dfn): + os.remove(dfn) + os.rename(sfn, dfn) + except FileNotFoundError: + # File was deleted between exists check and operation - ignore + pass + except OSError: + # Broad suppression intentional: logging must never crash. + # See module docstring for full rationale. + pass + + dfn = self.rotation_filename(f"{self.baseFilename}.1") + try: + if os.path.exists(dfn): + os.remove(dfn) + except FileNotFoundError: + # File was deleted between exists check and remove - ignore + pass + except OSError: + # Broad suppression intentional: logging must never crash. + # See module docstring for full rationale. + pass + + try: + self.rotate(self.baseFilename, dfn) + except FileNotFoundError: + # Base file was deleted - just create a new one + pass + except OSError: + # Broad suppression intentional: logging must never crash. + # See module docstring for full rationale. + pass + + if not self.delay: + try: + self.stream = self._open() + except OSError: + # Cannot open new log file - leave stream as None. + # FileHandler.emit() will attempt to open on next log entry. + pass diff --git a/webhook_server/utils/webhook.py b/webhook_server/utils/webhook.py index e54ee797a..a95030249 100644 --- a/webhook_server/utils/webhook.py +++ b/webhook_server/utils/webhook.py @@ -20,7 +20,7 @@ def process_github_webhook( webhook_ip: str, apis_dict: dict[str, dict[str, Any]], secret: str | None = None, -) -> tuple[bool, str, Callable]: +) -> tuple[bool, str, Callable[..., Any]]: full_repository_name: str = data["name"] github_api = apis_dict[repository_name].get("api") api_user = apis_dict[repository_name].get("user") diff --git a/webhook_server/web/log_viewer.py b/webhook_server/web/log_viewer.py index 425a0a1ab..4d0d0bffb 100644 --- a/webhook_server/web/log_viewer.py +++ b/webhook_server/web/log_viewer.py @@ -778,7 +778,7 @@ async def _stream_log_entries( # Sort log files to prioritize JSON webhook files first (primary data source), # then other files by modification time (newest first) # This ensures webhook data is displayed before internal log files - def sort_key(f: Path) -> tuple: + def sort_key(f: Path) -> tuple[int, float]: is_json_webhook = f.suffix == ".json" and f.name.startswith("webhooks_") # JSON webhook files: (0, -mtime) - highest priority, newest first # Other files: (1, -mtime) - lower priority, newest first