From 1f23d80759e4f9f02ad15a9477af0f72f36932b9 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 10:54:45 +0200 Subject: [PATCH 01/19] feat(labels): add configurable labels with enable/disable and custom colors - Add labels schema to schema.yaml (global + per-repo levels) - Add enabled_labels and label_colors config loading in github_api.py - Add is_label_enabled() method in labels_handler.py - Update _get_label_color() to use configured colors - Add validation for enabled-labels with warning for invalid categories - Fix infinite recursion bug in _get_custom_pr_size_thresholds() - Use ALL_LABELS_DICT for DEFAULT_LABEL_COLORS to eliminate duplication - Add comprehensive tests for label configuration - Update examples/config.yaml with labels examples Note: reviewed-by labels (approved-*, lgtm-*, etc.) are always enabled and cannot be disabled as they are the source of truth for the approval system. Closes #976 --- examples/config.yaml | 43 +++++ webhook_server/config/schema.yaml | 66 +++++++ webhook_server/libs/github_api.py | 26 +++ .../libs/handlers/labels_handler.py | 118 ++++++++++-- webhook_server/tests/test_config_schema.py | 37 ++++ webhook_server/tests/test_labels_handler.py | 173 ++++++++++++++++++ webhook_server/utils/constants.py | 19 ++ 7 files changed, 470 insertions(+), 12 deletions(-) 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/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/github_api.py b/webhook_server/libs/github_api.py index 15c7d30fe..c1a7bb2dc 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -29,6 +29,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, @@ -647,6 +648,31 @@ 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_config: dict[str, Any] = self.config.get_value("labels", return_on_none={}) or {} + repo_labels_config: dict[str, Any] = ( + self.config.get_value("labels", return_on_none={}, extra_dict=repository_config) or {} + ) + + # 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 + if "enabled-labels" in merged_labels_config: + enabled_set = set(merged_labels_config["enabled-labels"]) + # Log warning for invalid categories + invalid = enabled_set - CONFIGURABLE_LABEL_CATEGORIES + if invalid: + self.logger.warning( + f"{self.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: custom label colors (CSS3 color names) + self.label_colors: dict[str, str] = merged_labels_config.get("colors", {}) + 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/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index f1e01693b..4d6372532 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -12,15 +12,23 @@ ADD_STR, APPROVE_STR, APPROVED_BY_LABEL_PREFIX, + AUTOMERGE_LABEL_STR, + BRANCH_LABEL_PREFIX, + CAN_BE_MERGED_STR, CHANGED_REQUESTED_BY_LABEL_PREFIX, + CHERRY_PICK_LABEL_PREFIX, + CHERRY_PICKED_LABEL_PREFIX, COMMENTED_BY_LABEL_PREFIX, + DEFAULT_LABEL_COLORS, DELETE_STR, - DYNAMIC_LABELS_DICT, + HAS_CONFLICTS_LABEL_STR, HOLD_LABEL_STR, LGTM_BY_LABEL_PREFIX, LGTM_STR, + NEEDS_REBASE_LABEL_STR, SIZE_LABEL_PREFIX, STATIC_LABELS_DICT, + VERIFIED_LABEL_STR, WIP_STR, ) @@ -38,6 +46,66 @@ 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. + """ + # 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 + + enabled_labels = self.github_webhook.enabled_labels + + # If not configured, all labels are enabled + if enabled_labels is None: + return True + + # Map label to its category + label_to_category = { + VERIFIED_LABEL_STR: "verified", + HOLD_LABEL_STR: "hold", + WIP_STR: "wip", + NEEDS_REBASE_LABEL_STR: "needs-rebase", + HAS_CONFLICTS_LABEL_STR: "has-conflicts", + CAN_BE_MERGED_STR: "can-be-merged", + AUTOMERGE_LABEL_STR: "automerge", + } + + # Check static labels + if label in label_to_category: + return label_to_category[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_PREFIX: + 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) @@ -68,6 +136,10 @@ async def _add_label(self, pull_request: PullRequest, label: str) -> None: self.logger.debug(f"{label} is too long, not adding.") return + if not self.is_label_enabled(label): + self.logger.debug(f"{self.log_prefix} Label {label} is disabled by configuration, not adding") + 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 assign") return @@ -108,28 +180,42 @@ 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 + + # Direct match for static labels + if label in custom_colors: + return self._get_color_hex(custom_colors[label]) + + # Check prefix matches for dynamic labels + 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.""" @@ -186,7 +272,15 @@ 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 [ + (20, "XS", "ededed"), + (50, "S", "0E8A16"), + (100, "M", "F09C74"), + (300, "L", "F5621C"), + (500, "XL", "D93F0B"), + (float("inf"), "XXL", "B60205"), + ] return sorted_thresholds diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index d76947a7c..103561639 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -641,3 +641,40 @@ 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) diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index a8c2df4b7..ad3184698 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -51,6 +51,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 @@ -152,6 +156,20 @@ 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 + + await labels_handler._add_label(mock_pull_request, "hold") + + # Verify label was not added (disabled by config) + 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.""" @@ -1130,3 +1148,158 @@ 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") is True + assert labels_handler.is_label_enabled("hold") is True + assert labels_handler.is_label_enabled("wip") is True + assert labels_handler.is_label_enabled("size/M") is True + assert labels_handler.is_label_enabled("branch-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") is True + assert labels_handler.is_label_enabled("hold") is True + assert labels_handler.is_label_enabled("size/M") is True + assert labels_handler.is_label_enabled("wip") is False + assert labels_handler.is_label_enabled("needs-rebase") is False + assert labels_handler.is_label_enabled("branch-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("branch-main") is True + assert labels_handler.is_label_enabled("branch-feature-123") is True + assert labels_handler.is_label_enabled("verified") 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("cherry-pick-v1.0") is True + assert labels_handler.is_label_enabled("CherryPicked") is True + assert labels_handler.is_label_enabled("verified") 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") is False + assert labels_handler.is_label_enabled("hold") is False + assert labels_handler.is_label_enabled("wip") is False + assert labels_handler.is_label_enabled("size/M") is False + assert labels_handler.is_label_enabled("branch-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_automerge_label_category(self, labels_handler: LabelsHandler) -> None: + """Automerge label should be controlled by 'automerge' category.""" + labels_handler.github_webhook.enabled_labels = {"automerge"} + + assert labels_handler.is_label_enabled("automerge") is True + assert labels_handler.is_label_enabled("verified") 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("size/XS") is True + assert labels_handler.is_label_enabled("size/CustomName") is True + assert labels_handler.is_label_enabled("size/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/utils/constants.py b/webhook_server/utils/constants.py index 17ca4a00d..4c0ec13ba 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -75,6 +75,25 @@ ALL_LABELS_DICT: dict[str, str] = {**STATIC_LABELS_DICT, **DYNAMIC_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 +DEFAULT_LABEL_COLORS: dict[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: ok: str = "+1" From fbe147ddadb84d399f7336a2521e8d02df8d49a2 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 12:28:35 +0200 Subject: [PATCH 02/19] test(labels): add edge case tests for labels configuration Add validate_labels_config() method to Config class to validate enabled-labels against allowed categories. Add test cases for: - Invalid label category rejection - Empty enabled-labels array validation --- webhook_server/libs/config.py | 33 +++++++++++++++++ webhook_server/tests/test_config_schema.py | 41 ++++++++++++++++++++++ 2 files changed, 74 insertions(+) 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/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 103561639..596282d41 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -678,3 +678,44 @@ def test_labels_configuration_schema(self, monkeypatch: pytest.MonkeyPatch) -> N 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, Exception)): + 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) From 0e02c5c5e8679036f527d58e9656c92a78fc92f0 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 12:52:18 +0200 Subject: [PATCH 03/19] perf(clone): optimize git clone with shallow clone and targeted fetch Optimize repository cloning performance: - Use --depth 1 for shallow clone (skip full git history) - Replace git remote update with targeted fetch for specific PR/branch - For PRs: fetch only +refs/pull/{number}/head - For branches/tags: fetch only the specific ref Also fix overly broad exception handling in test_config_schema.py: - Use specific pytest.raises(ValueError, match=...) instead of catching (ValueError, Exception) --- webhook_server/libs/github_api.py | 44 ++++++------ webhook_server/tests/test_config_schema.py | 2 +- webhook_server/tests/test_github_api.py | 78 +++++++++++++++++++++- 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index c1a7bb2dc..1dff8f865 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -293,7 +293,7 @@ async def _clone_repository( clone_url_with_token = self.repository.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}", + command=f"git clone --depth 1 {clone_url_with_token} {self.clone_repo_dir}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.mask_sensitive, @@ -325,25 +325,29 @@ def redact_output(value: str) -> str: 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 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 what's needed instead of all refs + if pull_request: + # Fetch only this specific PR's ref + pr_number = await asyncio.to_thread(lambda: pull_request.number) + rc, _, _ = 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: + self.logger.warning(f"{self.log_prefix} Failed to fetch PR {pr_number} ref") + else: + # For push events (tags/branches), the ref is already cloned + # Just ensure we have the checkout_ref + if checkout_ref: + ref_to_fetch = checkout_ref.replace("refs/tags/", "").replace("refs/heads/", "") + rc, _, _ = await run_command( + command=f"{git_cmd} fetch origin {ref_to_fetch}", + log_prefix=self.log_prefix, + mask_sensitive=self.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Failed to fetch ref {ref_to_fetch}") # Determine checkout target if pull_request: diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 596282d41..91e058bdf 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -695,7 +695,7 @@ def test_labels_invalid_category_rejected(self, monkeypatch: pytest.MonkeyPatch) monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) # Invalid label category should raise a validation error - with pytest.raises((ValueError, Exception)): + with pytest.raises(ValueError, match=r"Invalid label categories in enabled-labels"): Config() 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..15283b88e 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1308,8 +1308,9 @@ async def mock_run_command(**kwargs: Any) -> tuple[bool, str, str]: command = kwargs.get("command", "") 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") + # Targeted PR fetch (replaced git remote update) + if "fetch origin +refs/pull/" in command: + return (False, "", "Fetch failed") return (True, "", "") mock_logger = Mock() @@ -1326,7 +1327,7 @@ 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) == 3 # user.name, user.email, PR fetch @pytest.mark.asyncio async def test_clone_repository_general_exception( @@ -1439,6 +1440,77 @@ 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( + self, + minimal_hook_data: dict, + minimal_headers: dict, + logger: Mock, + get_value_side_effect: Any, + ) -> None: + """Test _clone_repository with checkout_ref fetches and checks out the correct branch. + + Verifies that when checkout_ref="refs/heads/feature-branch" is provided: + 1. git fetch origin feature-branch is called + 2. git checkout feature-branch 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: Any) -> tuple[bool, str, str]: + executed_commands.append(command) + return (True, "", "") + + with patch( + "webhook_server.libs.github_api.run_command", + side_effect=mock_run_command, + ): + await gh._clone_repository(checkout_ref="refs/heads/feature-branch") + + # Verify clone succeeded + assert gh._repo_cloned is True + + # Find the fetch command for the branch + fetch_commands = [ + cmd for cmd in executed_commands if "fetch origin feature-branch" in cmd + ] + assert len(fetch_commands) == 1, ( + f"Expected exactly one fetch command for feature-branch, got: {fetch_commands}" + ) + + # Find the checkout command + checkout_commands = [ + cmd for cmd in executed_commands if "checkout feature-branch" in cmd + ] + assert len(checkout_commands) == 1, ( + f"Expected exactly one checkout command for feature-branch, " + f"got: {checkout_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") From a7583e43b955dc55f07dbe03f5a3a397192cdda8 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 13:06:58 +0200 Subject: [PATCH 04/19] fix(github-api): prevent AttributeError when log_prefix not yet assigned Use getattr() for safe access to log_prefix in _repo_data_from_config method, which may be called before log_prefix is initialized. --- webhook_server/libs/github_api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 1dff8f865..77b2605bb 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -668,8 +668,9 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: # Log warning for invalid categories invalid = enabled_set - CONFIGURABLE_LABEL_CATEGORIES if invalid: + log_prefix = getattr(self, "log_prefix", "") self.logger.warning( - f"{self.log_prefix} Invalid label categories in enabled-labels config: {invalid}. " + 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 From 9bcab8d7cb3406ecde1e646ea693ad7b6dcf84ad Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 13:30:53 +0200 Subject: [PATCH 05/19] fix(clone): fetch base branch before PR ref for non-default branch targets Ensures PRs targeting non-default branches (e.g., release-1.0 instead of main) work correctly with shallow clone by fetching the base branch before the PR ref. --- webhook_server/libs/github_api.py | 12 ++++ webhook_server/tests/test_github_api.py | 96 +++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 77b2605bb..cb05eac16 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -327,6 +327,18 @@ def redact_output(value: str) -> str: # 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 only this specific PR's ref pr_number = await asyncio.to_thread(lambda: pull_request.number) rc, _, _ = await run_command( diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 15283b88e..01cd17649 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1511,6 +1511,102 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str f"got: {checkout_commands}" ) + @pytest.mark.asyncio + async def test_clone_repository_fetches_base_branch_for_pr( + self, + minimal_hook_data: dict, + minimal_headers: dict, + logger: Mock, + get_value_side_effect: Any, + ) -> 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: Any) -> tuple[bool, str, str]: + executed_commands.append(command) + return (True, "", "") + + with patch( + "webhook_server.libs.github_api.run_command", + side_effect=mock_run_command, + ): + await gh._clone_repository(pull_request=mock_pr) + + # Verify clone succeeded + assert gh._repo_cloned is True + + # Find the base branch fetch command + base_fetch_commands = [ + cmd for cmd in executed_commands if "fetch origin release-1.0" in cmd + ] + assert len(base_fetch_commands) == 1, ( + f"Expected exactly one fetch command for base branch release-1.0, " + f"got: {base_fetch_commands}" + ) + + # Find the PR ref fetch command + pr_fetch_commands = [ + cmd + for cmd in executed_commands + if "fetch origin +refs/pull/123/head:refs/remotes/origin/pr/123" in cmd + ] + assert len(pr_fetch_commands) == 1, ( + f"Expected exactly one PR ref fetch command, got: {pr_fetch_commands}" + ) + + # Verify order: base branch fetch should come BEFORE PR ref fetch + base_fetch_index = next( + i for i, cmd in enumerate(executed_commands) if "fetch origin release-1.0" in cmd + ) + pr_fetch_index = next( + i + for i, cmd in enumerate(executed_commands) + if "fetch origin +refs/pull/123/head" 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") From 4f613ba430799356eb8dc88b40d6a52d8daf1305 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 13:43:18 +0200 Subject: [PATCH 06/19] feat(labels): add configurable labels with enable/disable and custom colors --- README.md | 89 +++++++ .../libs/handlers/issue_comment_handler.py | 36 ++- .../libs/handlers/labels_handler.py | 21 +- .../libs/handlers/pull_request_handler.py | 228 ++++++++++++++++-- webhook_server/tests/test_app_utils.py | 142 ++++++++++- .../tests/test_issue_comment_handler.py | 120 +++++++-- webhook_server/tests/test_labels_handler.py | 27 ++- .../tests/test_pull_request_handler.py | 125 ++++++++++ webhook_server/utils/app_utils.py | 23 +- webhook_server/utils/constants.py | 1 + 10 files changed, 723 insertions(+), 89 deletions(-) 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/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 7c33240fd..e86aba776 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,10 @@ async def user_commands( return await self.pull_request_handler.process_command_reprocess(pull_request=pull_request) + elif _command == COMMAND_REGENERATE_WELCOME_STR: + 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 +262,15 @@ 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) + await asyncio.to_thread(pull_request.edit, title=pr_title.replace(wip_for_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) + 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 +287,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( diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index 4d6372532..083cc93d2 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -129,26 +129,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 not self.is_label_enabled(label): self.logger.debug(f"{self.log_prefix} Label {label} is disabled by configuration, 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 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 + 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}" @@ -163,7 +164,7 @@ 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'}") diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 65880da27..50c72774f 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -14,6 +14,7 @@ from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.libs.handlers.runner_handler import RunnerHandler from webhook_server.utils.constants import ( + APPROVE_STR, APPROVED_BY_LABEL_PREFIX, AUTOMERGE_LABEL_STR, BRANCH_LABEL_PREFIX, @@ -29,6 +30,7 @@ HOLD_LABEL_STR, LABELS_SEPARATOR, LGTM_BY_LABEL_PREFIX, + LGTM_STR, NEEDS_REBASE_LABEL_STR, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, @@ -219,7 +221,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 +256,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 +274,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 +287,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 +301,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 +371,181 @@ 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" + + def _is_user_label_enabled(self, label_category: str) -> bool: + """Check if a user label category is enabled. + + Args: + label_category: The label category to check (e.g., "wip", "hold", "verified"). + + Returns: + True if the label is enabled, False otherwise. + Always returns True for non-configurable labels (lgtm, approve). + """ + # lgtm and approve are review labels - always enabled + if label_category in ("lgtm", "approve"): + return True + + enabled_labels = self.github_webhook.enabled_labels + + # If not configured, all labels are enabled + if enabled_labels is None: + return True + + return label_category in enabled_labels + + @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._is_user_label_enabled("wip"): + 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._is_user_label_enabled("hold"): + commands.append("* `/hold` - Block PR merging (approvers only)") + commands.append("* `/hold cancel` - Unblock PR merging") + + if self._is_user_label_enabled("verified"): + 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. + """ + # Mapping from USER_LABELS_DICT keys to their categories for filtering + label_to_category = { + 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 + } + + enabled_user_labels = [ + label + for label in USER_LABELS_DICT.keys() + if self._is_user_label_enabled(label_to_category.get(label, 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._is_user_label_enabled("wip"): + tips.append("* **WIP Status**: Use `/wip` when your PR is not ready for review") + + if self._is_user_label_enabled("verified"): + 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._is_cherry_pick_enabled(): + 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._is_user_label_enabled("wip"): + blockers.append("WIP") + + if self._is_user_label_enabled("hold"): + blockers.append("hold") + + # Conflict labels (has-conflicts) are always shown since they're fundamental + blockers.append("conflict") + + if not blockers: + return "4. **No Blockers**: No blocking labels present" + + return f"4. **No Blockers**: No {', '.join(blockers)} labels" + + def _is_cherry_pick_enabled(self) -> bool: + """Check if cherry-pick labels are enabled.""" + enabled_labels = self.github_webhook.enabled_labels + if enabled_labels is None: + return True + return "cherry-pick" in enabled_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._is_user_label_enabled("automerge"): + 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._is_cherry_pick_enabled(): + 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. @@ -1156,6 +1320,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/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_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 88185efe4..27d50ff7a 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, @@ -441,47 +442,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 +941,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 ad3184698..ae5403f3b 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -174,10 +174,19 @@ async def test_add_label_disabled_by_config(self, labels_handler: LabelsHandler, 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): + with patch("asyncio.to_thread") as mock_to_thread: + # get_label returns label, edit succeeds, add_to_labels succeeds + mock_label = Mock() + mock_to_thread.side_effect = [mock_label, None, None] + await labels_handler._add_label(mock_pull_request, static_label) + # Verify label_exists_in_pull_request was called + mock_exists.assert_called_once() + # Verify to_thread was called for: get_label, edit, add_to_labels + assert mock_to_thread.call_count == 3 @pytest.mark.asyncio async def test_add_label_exception_handling(self, labels_handler: LabelsHandler, mock_pull_request: Mock) -> None: @@ -315,15 +324,19 @@ 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) + # 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: diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 78a159294..eeb727f5f 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -81,6 +81,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 +403,38 @@ 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 + 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.""" + pull_request_handler.github_webhook.enabled_labels = {"hold", "verified"} + 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.""" + pull_request_handler.github_webhook.enabled_labels = {"wip", "verified"} + 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.""" + pull_request_handler.github_webhook.enabled_labels = {"verified"} + 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 @@ -2000,3 +2033,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/utils/app_utils.py b/webhook_server/utils/app_utils.py index dff57f25a..688e53ca6 100644 --- a/webhook_server/utils/app_utils.py +++ b/webhook_server/utils/app_utils.py @@ -176,21 +176,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 4c0ec13ba..85623b4ec 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -37,6 +37,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" From c716fcb5f5e53bf293f13b15887ae54d0838d0d6 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 14:05:28 +0200 Subject: [PATCH 07/19] refactor(labels): centralize label-category mapping and fix TimeoutWatch bug Add LABEL_CATEGORY_MAP constant to centralize label-to-category mapping, removing duplicate definition from pull_request_handler.py. Fix critical TimeoutWatch bug in labels_handler.py where instantiating inside the while condition caused infinite loops. Remove unreachable dead code from _prepare_no_blockers_requirement. Update tests to use constants instead of hardcoded strings and add missing return value assertion. Co-Authored-By: Claude Opus 4.5 --- .../libs/handlers/labels_handler.py | 3 +- .../libs/handlers/pull_request_handler.py | 18 +----- webhook_server/tests/test_labels_handler.py | 62 ++++++++++--------- webhook_server/utils/constants.py | 10 +++ 4 files changed, 48 insertions(+), 45 deletions(-) diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index 083cc93d2..54caa132c 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -168,7 +168,8 @@ async def _add_label(self, pull_request: PullRequest, label: str) -> bool: 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 diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 50c72774f..f6c33f02b 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -14,7 +14,6 @@ from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.libs.handlers.runner_handler import RunnerHandler from webhook_server.utils.constants import ( - APPROVE_STR, APPROVED_BY_LABEL_PREFIX, AUTOMERGE_LABEL_STR, BRANCH_LABEL_PREFIX, @@ -28,9 +27,9 @@ FAILURE_STR, HAS_CONFLICTS_LABEL_STR, HOLD_LABEL_STR, + LABEL_CATEGORY_MAP, LABELS_SEPARATOR, LGTM_BY_LABEL_PREFIX, - LGTM_STR, NEEDS_REBASE_LABEL_STR, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, @@ -442,20 +441,10 @@ def _prepare_available_labels_section(self) -> str: Only shows labels that are enabled. """ - # Mapping from USER_LABELS_DICT keys to their categories for filtering - label_to_category = { - 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 - } - enabled_user_labels = [ label for label in USER_LABELS_DICT.keys() - if self._is_user_label_enabled(label_to_category.get(label, label)) + if self._is_user_label_enabled(LABEL_CATEGORY_MAP.get(label, label)) ] if not enabled_user_labels: @@ -508,9 +497,6 @@ def _prepare_no_blockers_requirement(self) -> str: # Conflict labels (has-conflicts) are always shown since they're fundamental blockers.append("conflict") - if not blockers: - return "4. **No Blockers**: No blocking labels present" - return f"4. **No Blockers**: No {', '.join(blockers)} labels" def _is_cherry_pick_enabled(self) -> bool: diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index ae5403f3b..f19e8c984 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -9,10 +9,15 @@ from webhook_server.utils.constants import ( ADD_STR, APPROVE_STR, + AUTOMERGE_LABEL_STR, + BRANCH_LABEL_PREFIX, + CHERRY_PICK_LABEL_PREFIX, + CHERRY_PICKED_LABEL_PREFIX, HOLD_LABEL_STR, LGTM_STR, SIZE_LABEL_PREFIX, STATIC_LABELS_DICT, + VERIFIED_LABEL_STR, WIP_STR, ) @@ -165,9 +170,10 @@ async def test_add_label_disabled_by_config(self, labels_handler: LabelsHandler, with patch.object(labels_handler, "label_exists_in_pull_request", new_callable=AsyncMock) as mock_exists: mock_exists.return_value = False - await labels_handler._add_label(mock_pull_request, "hold") + 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 @@ -1188,11 +1194,11 @@ def test_all_labels_enabled_when_not_configured(self, labels_handler: LabelsHand """When enabled_labels is None, all labels should be enabled.""" labels_handler.github_webhook.enabled_labels = None - assert labels_handler.is_label_enabled("verified") is True - assert labels_handler.is_label_enabled("hold") is True - assert labels_handler.is_label_enabled("wip") is True - assert labels_handler.is_label_enabled("size/M") is True - assert labels_handler.is_label_enabled("branch-main") is True + 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.""" @@ -1208,28 +1214,28 @@ 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") is True - assert labels_handler.is_label_enabled("hold") is True - assert labels_handler.is_label_enabled("size/M") is True - assert labels_handler.is_label_enabled("wip") is False + 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("branch-main") 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("branch-main") is True - assert labels_handler.is_label_enabled("branch-feature-123") is True - assert labels_handler.is_label_enabled("verified") is False + 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("cherry-pick-v1.0") is True - assert labels_handler.is_label_enabled("CherryPicked") is True - assert labels_handler.is_label_enabled("verified") is False + assert labels_handler.is_label_enabled(f"{CHERRY_PICK_LABEL_PREFIX}v1.0") is True + assert labels_handler.is_label_enabled(CHERRY_PICKED_LABEL_PREFIX) 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.""" @@ -1242,29 +1248,29 @@ def test_empty_enabled_labels_disables_all_except_reviewed_by(self, labels_handl """Empty enabled_labels should disable all configurable labels.""" labels_handler.github_webhook.enabled_labels = set() - assert labels_handler.is_label_enabled("verified") is False - assert labels_handler.is_label_enabled("hold") is False - assert labels_handler.is_label_enabled("wip") is False - assert labels_handler.is_label_enabled("size/M") is False - assert labels_handler.is_label_enabled("branch-main") is False + 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_automerge_label_category(self, labels_handler: LabelsHandler) -> None: """Automerge label should be controlled by 'automerge' category.""" - labels_handler.github_webhook.enabled_labels = {"automerge"} + labels_handler.github_webhook.enabled_labels = {AUTOMERGE_LABEL_STR} - assert labels_handler.is_label_enabled("automerge") is True - assert labels_handler.is_label_enabled("verified") is False + 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("size/XS") is True - assert labels_handler.is_label_enabled("size/CustomName") is True - assert labels_handler.is_label_enabled("size/Massive") is True + 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: diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 85623b4ec..84ca5d94d 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -51,6 +51,16 @@ AUTOMERGE_LABEL_STR: "0E8A16", } +# Mapping from label strings to their configuration category names +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 +} + STATIC_LABELS_DICT: dict[str, str] = { **USER_LABELS_DICT, CHERRY_PICKED_LABEL_PREFIX: "1D76DB", From 5511d030e042f4b2dfe05283aa671b50b6335d17 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 14:18:11 +0200 Subject: [PATCH 08/19] refactor(labels): extract STATIC_PR_SIZE_THRESHOLDS constant Eliminate duplication of static PR size threshold defaults by extracting them into a module-level constant. --- .../libs/handlers/labels_handler.py | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index 54caa132c..b50d33839 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -35,6 +35,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: @@ -242,14 +252,7 @@ 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) thresholds = [] for label_name, config in custom_config.items(): @@ -275,14 +278,7 @@ 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 static defaults directly to avoid infinite recursion - 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) return sorted_thresholds From ae34aaddd75f44ef65664b81f747531d4e7b48b4 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 15:07:01 +0200 Subject: [PATCH 09/19] fix(owners): handle no merge base error with fallback to two-dot diff When git diff fails with "no merge base" (e.g., force push, fork with divergent history), fall back to two-dot diff syntax instead of crashing. --- .../libs/handlers/owners_files_handler.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/webhook_server/libs/handlers/owners_files_handler.py b/webhook_server/libs/handlers/owners_files_handler.py index 77f1e49d6..d608916ba 100644 --- a/webhook_server/libs/handlers/owners_files_handler.py +++ b/webhook_server/libs/handlers/owners_files_handler.py @@ -107,6 +107,7 @@ 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) git_diff_command = ( f"git -C {shlex.quote(self.github_webhook.clone_repo_dir)} diff --name-only {base_sha}...{head_sha}" ) @@ -119,6 +120,19 @@ 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") + 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 = ( From f164ec6eb33f83849de3f60eb0b1236b8ead5f74 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 15:13:08 +0200 Subject: [PATCH 10/19] fix(labels): handle misconfigured label_colors as list instead of dict Add type validation to _get_label_color to gracefully handle cases where label_colors is configured as a list instead of dict. --- webhook_server/libs/handlers/labels_handler.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index b50d33839..d8408f423 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -197,6 +197,9 @@ def _get_label_color(self, label: str) -> str: """ # 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: From 8474f77d0580df1dde600dd7e96cdf74c09eab12 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 15:39:11 +0200 Subject: [PATCH 11/19] fix(handlers): improve type safety and code clarity Add first-match-wins comment for prefix color matching. Fix misleading error message to show correct diff syntax after fallback. Remove unnecessary None checks for PyGithub int properties. Add type validation at source in github_api.py. Fix generic type parameters across codebase. --- .pre-commit-config.yaml | 3 +- pyproject.toml | 2 +- scripts/generate_changelog.py | 11 ++-- webhook_server/app.py | 7 ++- webhook_server/libs/github_api.py | 58 +++++++++++-------- .../libs/handlers/check_run_handler.py | 2 +- .../libs/handlers/issue_comment_handler.py | 2 +- .../libs/handlers/labels_handler.py | 32 ++++++++-- .../libs/handlers/owners_files_handler.py | 8 ++- webhook_server/tests/test_labels_handler.py | 32 +--------- webhook_server/utils/app_utils.py | 3 +- .../github_repository_and_webhook_settings.py | 2 +- .../utils/github_repository_settings.py | 8 +-- webhook_server/utils/helpers.py | 2 +- webhook_server/utils/webhook.py | 2 +- webhook_server/web/log_viewer.py | 2 +- 16 files changed, 94 insertions(+), 82 deletions(-) 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/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..e7963435d 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,7 +60,7 @@ 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) @@ -68,7 +69,9 @@ def parse_commit_line(line: str) -> dict: 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 @@ -80,7 +83,7 @@ def categorize_commit(commit: dict, title_to_type_map: dict, default_category: s return 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/libs/github_api.py b/webhook_server/libs/github_api.py index cb05eac16..df1fc3fa5 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 @@ -83,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"] @@ -603,17 +604,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"] @@ -639,19 +641,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 ) @@ -665,30 +670,33 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: ) # Load labels configuration - global_labels_config: dict[str, Any] = self.config.get_value("labels", return_on_none={}) or {} - repo_labels_config: dict[str, Any] = ( - self.config.get_value("labels", return_on_none={}, extra_dict=repository_config) or {} - ) + _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 - if "enabled-labels" in merged_labels_config: - enabled_set = set(merged_labels_config["enabled-labels"]) - # 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 + _enabled_labels = merged_labels_config.get("enabled-labels") + if _enabled_labels is not None: + if isinstance(_enabled_labels, list): + enabled_set = set(_enabled_labels) + # 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: custom label colors (CSS3 color names) - self.label_colors: dict[str, str] = merged_labels_config.get("colors", {}) + _colors = merged_labels_config.get("colors", {}) + self.label_colors: dict[str, str] = _colors if isinstance(_colors, dict) else {} self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index 50af9bbe0..78f82d174 100644 --- a/webhook_server/libs/handlers/check_run_handler.py +++ b/webhook_server/libs/handlers/check_run_handler.py @@ -286,7 +286,7 @@ 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"): + if self.github_webhook.pypi: _output = _output.replace(self.github_webhook.pypi["token"], _hased_str) if getattr(self.github_webhook, "container_repository_username", None): diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index e86aba776..329a0b108 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -417,7 +417,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 d8408f423..323c068de 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -86,6 +86,14 @@ def is_label_enabled(self, label: str) -> bool: if enabled_labels is None: return True + # Validate enabled_labels is a set (could be misconfigured) + if not isinstance(enabled_labels, set): + self.logger.warning( + f"{self.log_prefix} enabled_labels is not a set (got {type(enabled_labels).__name__}), " + "treating as all labels enabled" + ) + return True + # Map label to its category label_to_category = { VERIFIED_LABEL_STR: "verified", @@ -206,6 +214,8 @@ def _get_label_color(self, label: str) -> str: 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) @@ -257,8 +267,24 @@ def _get_custom_pr_size_thresholds(self) -> list[tuple[int | float, str, str]]: if not custom_config: 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 @@ -287,10 +313,8 @@ def _get_custom_pr_size_thresholds(self) -> list[tuple[int | float, str, str]]: 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 = pull_request.additions + deletions = pull_request.deletions size = additions + deletions self.logger.debug(f"{self.log_prefix} PR size is {size} (additions: {additions}, deletions: {deletions})") diff --git a/webhook_server/libs/handlers/owners_files_handler.py b/webhook_server/libs/handlers/owners_files_handler.py index d608916ba..8e4409e90 100644 --- a/webhook_server/libs/handlers/owners_files_handler.py +++ b/webhook_server/libs/handlers/owners_files_handler.py @@ -108,6 +108,7 @@ 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}" ) @@ -123,6 +124,7 @@ async def list_changed_files(self, pull_request: PullRequest) -> list[str]: # 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}" ) @@ -136,7 +138,7 @@ async def list_changed_files(self, pull_request: PullRequest) -> list[str]: # 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)'}" ) @@ -159,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/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index f19e8c984..e486245b3 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -23,7 +23,7 @@ class MockPullRequest: - def __init__(self, additions: int | None = 50, deletions: int | None = 10): + def __init__(self, additions: int = 50, deletions: int = 10): self.additions = additions self.deletions = deletions self.number = 123 @@ -103,36 +103,6 @@ def test_get_size_calculation( 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.""" diff --git a/webhook_server/utils/app_utils.py b/webhook_server/utils/app_utils.py index 688e53ca6..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: diff --git a/webhook_server/utils/github_repository_and_webhook_settings.py b/webhook_server/utils/github_repository_and_webhook_settings.py index d5caa3f85..70984e3a6 100644 --- a/webhook_server/utils/github_repository_and_webhook_settings.py +++ b/webhook_server/utils/github_repository_and_webhook_settings.py @@ -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[Any] = [] with ThreadPoolExecutor() as executor: for repo, _ in config.root_data["repositories"].items(): apis.append( 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..9d4dcede6 100644 --- a/webhook_server/utils/helpers.py +++ b/webhook_server/utils/helpers.py @@ -511,7 +511,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/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 From 295131ad2e3a2e598abaaf3a379a250390e4056c Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 16:02:09 +0200 Subject: [PATCH 12/19] fix: address coderabbit review comments for configurable labels --- scripts/generate_changelog.py | 9 ++--- webhook_server/libs/github_api.py | 34 ++++++++++++++----- .../libs/handlers/check_run_handler.py | 7 ++-- .../libs/handlers/issue_comment_handler.py | 4 +++ .../libs/handlers/labels_handler.py | 12 ++++--- webhook_server/tests/test_github_api.py | 7 ++-- webhook_server/tests/test_labels_handler.py | 29 +++++++++------- .../tests/test_pull_request_size.py | 5 +-- .../github_repository_and_webhook_settings.py | 4 +-- 9 files changed, 71 insertions(+), 40 deletions(-) diff --git a/scripts/generate_changelog.py b/scripts/generate_changelog.py index e7963435d..9228d015e 100644 --- a/scripts/generate_changelog.py +++ b/scripts/generate_changelog.py @@ -64,7 +64,7 @@ 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 {} @@ -76,11 +76,8 @@ def categorize_commit( 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[str, Any], section: str) -> str: diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index df1fc3fa5..4113cbdc3 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -350,17 +350,24 @@ def redact_output(value: str) -> str: if not rc: self.logger.warning(f"{self.log_prefix} Failed to fetch PR {pr_number} ref") else: - # For push events (tags/branches), the ref is already cloned - # Just ensure we have the checkout_ref + # For push events (tags/branches), use explicit refspecs to avoid ambiguity if checkout_ref: - ref_to_fetch = checkout_ref.replace("refs/tags/", "").replace("refs/heads/", "") + if checkout_ref.startswith("refs/tags/"): + tag_name = checkout_ref.replace("refs/tags/", "") + fetch_refspec = f"refs/tags/{tag_name}:refs/tags/{tag_name}" + elif checkout_ref.startswith("refs/heads/"): + branch_name = checkout_ref.replace("refs/heads/", "") + fetch_refspec = f"refs/heads/{branch_name}:refs/remotes/origin/{branch_name}" + else: + # Fallback for unexpected ref formats + fetch_refspec = checkout_ref rc, _, _ = await run_command( - command=f"{git_cmd} fetch origin {ref_to_fetch}", + 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 ref {ref_to_fetch}") + self.logger.warning(f"{self.log_prefix} Failed to fetch ref {checkout_ref}") # Determine checkout target if pull_request: @@ -683,7 +690,12 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: _enabled_labels = merged_labels_config.get("enabled-labels") if _enabled_labels is not None: if isinstance(_enabled_labels, list): - enabled_set = set(_enabled_labels) + # 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: + log_prefix = getattr(self, "log_prefix", "") + self.logger.warning(f"{log_prefix} Non-string entries in enabled-labels were ignored: {dropped}") # Log warning for invalid categories invalid = enabled_set - CONFIGURABLE_LABEL_CATEGORIES if invalid: @@ -694,9 +706,13 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: ) self.enabled_labels = enabled_set & CONFIGURABLE_LABEL_CATEGORIES # Only keep valid categories - # colors: custom label colors (CSS3 color names) - _colors = merged_labels_config.get("colors", {}) - self.label_colors: dict[str, str] = _colors if isinstance(_colors, dict) else {} + # 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) diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index 78f82d174..71028a194 100644 --- a/webhook_server/libs/handlers/check_run_handler.py +++ b/webhook_server/libs/handlers/check_run_handler.py @@ -286,8 +286,11 @@ def get_check_run_text(self, err: str, out: str) -> str: _hased_str = "*****" - if self.github_webhook.pypi: - _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 329a0b108..1cc04f323 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -241,6 +241,10 @@ async def user_commands( 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) diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index 323c068de..fa2688559 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -164,7 +164,7 @@ async def _add_label(self, pull_request: PullRequest, label: str) -> bool: 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") + self.logger.debug(f"{self.log_prefix} Label {label} already assigned") return False # Get the color for this label (custom or default) @@ -311,10 +311,12 @@ def _get_custom_pr_size_thresholds(self) -> list[tuple[int | float, str, str]]: 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.""" - additions = pull_request.additions - deletions = pull_request.deletions + 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})") @@ -336,7 +338,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/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 01cd17649..f19fcb6a1 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1494,9 +1494,12 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str # Verify clone succeeded assert gh._repo_cloned is True - # Find the fetch command for the branch + # Find the fetch command for the branch (uses explicit refspec format) fetch_commands = [ - cmd for cmd in executed_commands if "fetch origin feature-branch" in cmd + cmd + for cmd in executed_commands + if "fetch origin refs/heads/feature-branch:refs/remotes/origin/feature-branch" + in cmd ] assert len(fetch_commands) == 1, ( f"Expected exactly one fetch command for feature-branch, got: {fetch_commands}" diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index e486245b3..868c45c68 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -23,7 +23,7 @@ class MockPullRequest: - def __init__(self, additions: int = 50, deletions: int = 10): + def __init__(self, additions: int = 50, deletions: int = 10) -> None: self.additions = additions self.deletions = deletions self.number = 123 @@ -79,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", [ @@ -91,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.""" @@ -99,7 +100,7 @@ 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}" @@ -442,7 +443,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) @@ -461,7 +463,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}" ) @@ -577,7 +579,7 @@ 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, "get_size", new_callable=AsyncMock, return_value=None): with patch.object(labels_handler, "_add_label") as mock_add: await labels_handler.add_size_label(mock_pull_request) mock_add.assert_not_called() @@ -889,7 +891,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 = { @@ -913,10 +916,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 = { @@ -936,7 +940,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: @@ -1092,7 +1096,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 = { @@ -1117,7 +1122,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}" ) 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/utils/github_repository_and_webhook_settings.py b/webhook_server/utils/github_repository_and_webhook_settings.py index 70984e3a6..9c6faee06 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[Any] = [] + apis: list[Future[tuple[str, github.Github | None, str]]] = [] with ThreadPoolExecutor() as executor: for repo, _ in config.root_data["repositories"].items(): apis.append( From cd18c055402f711fc8b2e66377ff79c048f803d1 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 16:54:20 +0200 Subject: [PATCH 13/19] fix: address coderabbit review comments for configurable labels - Make WIP prefix handling idempotent (do not prepend if already present) - Rename CHERRY_PICKED_LABEL_PREFIX to CHERRY_PICKED_LABEL for clarity - Add asyncio.to_thread stubbing in clone tests for proper async handling - Move as_completed loop inside ThreadPoolExecutor context manager - Consolidate duplicate label_to_category mappings into LABEL_CATEGORY_MAP - Add tests for WIP idempotent behavior (add and remove cases) Co-Authored-By: Claude Opus 4.5 --- .../libs/handlers/check_run_handler.py | 12 ++-- .../libs/handlers/issue_comment_handler.py | 13 +++- .../libs/handlers/labels_handler.py | 27 ++------ .../libs/handlers/pull_request_handler.py | 4 +- .../libs/handlers/runner_handler.py | 8 +-- .../tests/test_check_run_handler.py | 8 +-- webhook_server/tests/test_github_api.py | 20 ++++-- .../tests/test_issue_comment_handler.py | 66 +++++++++++++++++++ webhook_server/tests/test_labels_handler.py | 4 +- .../tests/test_pull_request_handler.py | 6 +- webhook_server/utils/constants.py | 7 +- .../github_repository_and_webhook_settings.py | 6 +- 12 files changed, 124 insertions(+), 57 deletions(-) diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index 71028a194..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, diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 1cc04f323..650cc57aa 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -265,16 +265,25 @@ async def user_commands( elif _command == WIP_STR: wip_for_title: str = f"{WIP_STR.upper()}:" + wip_for_title_with_space: str = f"{wip_for_title} " if remove: 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) - await asyncio.to_thread(pull_request.edit, title=pr_title.replace(wip_for_title, "")) + # Use slicing to remove only the leading prefix (handles both "WIP:" and "WIP: " forms) + if pr_title.startswith(wip_for_title_with_space): + new_title = pr_title[len(wip_for_title_with_space) :] + await asyncio.to_thread(pull_request.edit, title=new_title) + elif pr_title.startswith(wip_for_title): + new_title = pr_title[len(wip_for_title) :] + await asyncio.to_thread(pull_request.edit, title=new_title) else: 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) - await asyncio.to_thread(pull_request.edit, title=f"{wip_for_title} {pr_title}") + # Only prepend if prefix is not already there (idempotent) + if not pr_title.startswith(wip_for_title): + 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: diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index fa2688559..09090107c 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -12,23 +12,19 @@ ADD_STR, APPROVE_STR, APPROVED_BY_LABEL_PREFIX, - AUTOMERGE_LABEL_STR, BRANCH_LABEL_PREFIX, - CAN_BE_MERGED_STR, CHANGED_REQUESTED_BY_LABEL_PREFIX, CHERRY_PICK_LABEL_PREFIX, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, COMMENTED_BY_LABEL_PREFIX, DEFAULT_LABEL_COLORS, DELETE_STR, - HAS_CONFLICTS_LABEL_STR, HOLD_LABEL_STR, + LABEL_CATEGORY_MAP, LGTM_BY_LABEL_PREFIX, LGTM_STR, - NEEDS_REBASE_LABEL_STR, SIZE_LABEL_PREFIX, STATIC_LABELS_DICT, - VERIFIED_LABEL_STR, WIP_STR, ) @@ -94,20 +90,9 @@ def is_label_enabled(self, label: str) -> bool: ) return True - # Map label to its category - label_to_category = { - VERIFIED_LABEL_STR: "verified", - HOLD_LABEL_STR: "hold", - WIP_STR: "wip", - NEEDS_REBASE_LABEL_STR: "needs-rebase", - HAS_CONFLICTS_LABEL_STR: "has-conflicts", - CAN_BE_MERGED_STR: "can-be-merged", - AUTOMERGE_LABEL_STR: "automerge", - } - - # Check static labels - if label in label_to_category: - return label_to_category[label] in enabled_labels + # 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): @@ -118,7 +103,7 @@ def is_label_enabled(self, label: str) -> bool: return "branch" in enabled_labels # Check cherry-pick labels - if label.startswith(CHERRY_PICK_LABEL_PREFIX) or label == CHERRY_PICKED_LABEL_PREFIX: + if label.startswith(CHERRY_PICK_LABEL_PREFIX) or label == CHERRY_PICKED_LABEL: return "cherry-pick" in enabled_labels # Unknown labels are allowed by default diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index f6c33f02b..750fd30e7 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, @@ -1031,7 +1031,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: 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_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_github_api.py b/webhook_server/tests/test_github_api.py index f19fcb6a1..96532bd33 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1447,6 +1447,7 @@ async def test_clone_repository_checkout_ref_fetch_path( minimal_headers: dict, logger: Mock, get_value_side_effect: Any, + to_thread_sync: Any, ) -> None: """Test _clone_repository with checkout_ref fetches and checks out the correct branch. @@ -1485,9 +1486,12 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str executed_commands.append(command) return (True, "", "") - with patch( - "webhook_server.libs.github_api.run_command", - side_effect=mock_run_command, + 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/heads/feature-branch") @@ -1521,6 +1525,7 @@ async def test_clone_repository_fetches_base_branch_for_pr( minimal_headers: dict, logger: Mock, get_value_side_effect: Any, + to_thread_sync: Any, ) -> None: """Test _clone_repository fetches base branch before PR ref when pull_request is provided. @@ -1567,9 +1572,12 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str executed_commands.append(command) return (True, "", "") - with patch( - "webhook_server.libs.github_api.run_command", - side_effect=mock_run_command, + 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) diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 27d50ff7a..28d8506ab 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -424,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.""" diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index 868c45c68..80dd46045 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -12,7 +12,7 @@ AUTOMERGE_LABEL_STR, BRANCH_LABEL_PREFIX, CHERRY_PICK_LABEL_PREFIX, - CHERRY_PICKED_LABEL_PREFIX, + CHERRY_PICKED_LABEL, HOLD_LABEL_STR, LGTM_STR, SIZE_LABEL_PREFIX, @@ -1209,7 +1209,7 @@ def test_cherry_pick_labels_category(self, labels_handler: LabelsHandler) -> Non 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_PREFIX) 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: diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index eeb727f5f..d4d87365b 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -14,7 +14,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, HAS_CONFLICTS_LABEL_STR, LGTM_BY_LABEL_PREFIX, @@ -683,7 +683,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 ( @@ -704,7 +704,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 ( diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 84ca5d94d..52c75d0e1 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -18,7 +18,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}" @@ -59,11 +59,14 @@ 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", diff --git a/webhook_server/utils/github_repository_and_webhook_settings.py b/webhook_server/utils/github_repository_and_webhook_settings.py index 9c6faee06..7664bd0b7 100644 --- a/webhook_server/utils/github_repository_and_webhook_settings.py +++ b/webhook_server/utils/github_repository_and_webhook_settings.py @@ -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}") From eb4b63b88354a1f68f4ddef9bcbfe5ce798c42ae Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 17:25:28 +0200 Subject: [PATCH 14/19] fix: address CodeRabbit review comments for configurable labels - Make WIP title prefix handling case-insensitive - Remove duplicate label enablement logic in PullRequestHandler (now uses labels_handler.is_label_enabled() consistently) - Fix brittle test assertions for asyncio.to_thread call counts in test_labels_handler.py - Fix test method calls for _prepare_no_blockers_requirement - Add clarifying comments for LABEL_CATEGORY_MAP explaining exact-match vs prefix-based label handling - Use MappingProxyType for DEFAULT_LABEL_COLORS to prevent mutation - Optimize push events to only clone for tags (branch pushes skip cloning since PushHandler only processes tags for PyPI/container builds) Co-Authored-By: Claude Opus 4.5 --- webhook_server/libs/github_api.py | 59 +++++++------- .../libs/handlers/issue_comment_handler.py | 16 ++-- .../libs/handlers/pull_request_handler.py | 54 +++---------- webhook_server/tests/test_github_api.py | 78 ++++++++++--------- webhook_server/tests/test_labels_handler.py | 23 +++--- .../tests/test_pull_request_handler.py | 14 +++- webhook_server/utils/constants.py | 20 ++++- 7 files changed, 134 insertions(+), 130 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 4113cbdc3..8d3aba5d7 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -350,34 +350,27 @@ def redact_output(value: str) -> str: if not rc: self.logger.warning(f"{self.log_prefix} Failed to fetch PR {pr_number} ref") else: - # For push events (tags/branches), use explicit refspecs to avoid ambiguity - if checkout_ref: - if checkout_ref.startswith("refs/tags/"): - tag_name = checkout_ref.replace("refs/tags/", "") - fetch_refspec = f"refs/tags/{tag_name}:refs/tags/{tag_name}" - elif checkout_ref.startswith("refs/heads/"): - branch_name = checkout_ref.replace("refs/heads/", "") - fetch_refspec = f"refs/heads/{branch_name}:refs/remotes/origin/{branch_name}" - else: - # Fallback for unexpected ref formats - fetch_refspec = checkout_ref - 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 ref {checkout_ref}") + # 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 + tag_name = checkout_ref.replace("refs/tags/", "") + 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/", "") # Checkout target branch/tag rc, _, err = await run_command( @@ -439,14 +432,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 diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 650cc57aa..a55f02a15 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -265,24 +265,24 @@ async def user_commands( elif _command == WIP_STR: wip_for_title: str = f"{WIP_STR.upper()}:" - wip_for_title_with_space: str = f"{wip_for_title} " if remove: 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) - # Use slicing to remove only the leading prefix (handles both "WIP:" and "WIP: " forms) - if pr_title.startswith(wip_for_title_with_space): - new_title = pr_title[len(wip_for_title_with_space) :] + # 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.startswith(wip_for_title): - new_title = pr_title[len(wip_for_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: 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) - # Only prepend if prefix is not already there (idempotent) - if not pr_title.startswith(wip_for_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: diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 750fd30e7..2b4c05ca1 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -27,7 +27,6 @@ FAILURE_STR, HAS_CONFLICTS_LABEL_STR, HOLD_LABEL_STR, - LABEL_CATEGORY_MAP, LABELS_SEPARATOR, LGTM_BY_LABEL_PREFIX, NEEDS_REBASE_LABEL_STR, @@ -384,28 +383,6 @@ def _prepare_labels_config_welcome_section(self) -> str: enabled_list = ", ".join(f"`{label}`" for label in sorted(enabled_labels)) return f"* **Labels**: Enabled categories: {enabled_list}\n" - def _is_user_label_enabled(self, label_category: str) -> bool: - """Check if a user label category is enabled. - - Args: - label_category: The label category to check (e.g., "wip", "hold", "verified"). - - Returns: - True if the label is enabled, False otherwise. - Always returns True for non-configurable labels (lgtm, approve). - """ - # lgtm and approve are review labels - always enabled - if label_category in ("lgtm", "approve"): - return True - - enabled_labels = self.github_webhook.enabled_labels - - # If not configured, all labels are enabled - if enabled_labels is None: - return True - - return label_category in enabled_labels - @property def _prepare_pr_status_commands_section(self) -> str: """Prepare the PR Status Management commands section for the welcome comment. @@ -414,15 +391,15 @@ def _prepare_pr_status_commands_section(self) -> str: """ commands: list[str] = [] - if self._is_user_label_enabled("wip"): + 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._is_user_label_enabled("hold"): + 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._is_user_label_enabled("verified"): + if self.labels_handler.is_label_enabled(VERIFIED_LABEL_STR): commands.append("* `/verified` - Mark PR as verified") commands.append("* `/verified cancel` - Remove verification status") @@ -442,9 +419,7 @@ def _prepare_available_labels_section(self) -> str: Only shows labels that are enabled. """ enabled_user_labels = [ - label - for label in USER_LABELS_DICT.keys() - if self._is_user_label_enabled(LABEL_CATEGORY_MAP.get(label, label)) + label for label in USER_LABELS_DICT.keys() if self.labels_handler.is_label_enabled(label) ] if not enabled_user_labels: @@ -460,14 +435,14 @@ def _prepare_tips_section(self) -> str: """ tips: list[str] = [] - if self._is_user_label_enabled("wip"): + 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._is_user_label_enabled("verified"): + 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._is_cherry_pick_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 @@ -488,10 +463,10 @@ def _prepare_no_blockers_requirement(self) -> str: """ blockers: list[str] = [] - if self._is_user_label_enabled("wip"): + if self.labels_handler.is_label_enabled(WIP_STR): blockers.append("WIP") - if self._is_user_label_enabled("hold"): + if self.labels_handler.is_label_enabled(HOLD_LABEL_STR): blockers.append("hold") # Conflict labels (has-conflicts) are always shown since they're fundamental @@ -499,20 +474,13 @@ def _prepare_no_blockers_requirement(self) -> str: return f"4. **No Blockers**: No {', '.join(blockers)} labels" - def _is_cherry_pick_enabled(self) -> bool: - """Check if cherry-pick labels are enabled.""" - enabled_labels = self.github_webhook.enabled_labels - if enabled_labels is None: - return True - return "cherry-pick" in enabled_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._is_user_label_enabled("automerge"): + 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" @@ -525,7 +493,7 @@ def _prepare_cherry_pick_section(self) -> str: Only shows the section if cherry-pick labels are enabled. """ - if self._is_cherry_pick_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` diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 96532bd33..a6514da83 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -44,10 +44,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"}}], } @@ -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] @@ -1441,7 +1441,7 @@ async def test_clone_repository_empty_checkout_ref( await webhook._clone_repository(checkout_ref="") @pytest.mark.asyncio - async def test_clone_repository_checkout_ref_fetch_path( + async def test_clone_repository_checkout_ref_fetch_path_for_tag( self, minimal_hook_data: dict, minimal_headers: dict, @@ -1449,11 +1449,13 @@ async def test_clone_repository_checkout_ref_fetch_path( get_value_side_effect: Any, to_thread_sync: Any, ) -> None: - """Test _clone_repository with checkout_ref fetches and checks out the correct branch. + """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/heads/feature-branch" is provided: - 1. git fetch origin feature-branch is called - 2. git checkout feature-branch is called + 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 @@ -1493,29 +1495,25 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str ), patch("asyncio.to_thread", side_effect=to_thread_sync), ): - await gh._clone_repository(checkout_ref="refs/heads/feature-branch") + await gh._clone_repository(checkout_ref="refs/tags/v1.0.0") # Verify clone succeeded assert gh._repo_cloned is True - # Find the fetch command for the branch (uses explicit refspec format) + # Find the fetch command for the tag (uses explicit refspec format) fetch_commands = [ cmd for cmd in executed_commands - if "fetch origin refs/heads/feature-branch:refs/remotes/origin/feature-branch" - in cmd + if "fetch origin refs/tags/v1.0.0:refs/tags/v1.0.0" in cmd ] assert len(fetch_commands) == 1, ( - f"Expected exactly one fetch command for feature-branch, got: {fetch_commands}" + f"Expected exactly one fetch command for tag v1.0.0, got: {fetch_commands}" ) - # Find the checkout command - checkout_commands = [ - cmd for cmd in executed_commands if "checkout feature-branch" in cmd - ] + # Find the checkout command (tag name without refs/tags/ prefix) + checkout_commands = [cmd for cmd in executed_commands if "checkout v1.0.0" in cmd] assert len(checkout_commands) == 1, ( - f"Expected exactly one checkout command for feature-branch, " - f"got: {checkout_commands}" + f"Expected exactly one checkout command for v1.0.0, got: {checkout_commands}" ) @pytest.mark.asyncio @@ -1704,7 +1702,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, @@ -1714,18 +1712,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"}}], } @@ -1743,22 +1744,25 @@ 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 NOT called + mock_process_push.assert_not_called() - # Verify PushHandler.process_push_webhook_data was called - mock_process_push.assert_called_once() + # Verify return value is None + assert result is None @pytest.mark.asyncio async def test_cleanup( diff --git a/webhook_server/tests/test_labels_handler.py b/webhook_server/tests/test_labels_handler.py index 80dd46045..af5e0950e 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -155,15 +155,20 @@ async def test_add_label_static_label(self, labels_handler: LabelsHandler, mock_ 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): - with patch("asyncio.to_thread") as mock_to_thread: - # get_label returns label, edit succeeds, add_to_labels succeeds - mock_label = Mock() - mock_to_thread.side_effect = [mock_label, None, None] - await labels_handler._add_label(mock_pull_request, static_label) - # Verify label_exists_in_pull_request was called - mock_exists.assert_called_once() - # Verify to_thread was called for: get_label, edit, add_to_labels - assert mock_to_thread.call_count == 3 + # 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: diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index d4d87365b..5590610dc 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -17,6 +17,7 @@ CHERRY_PICKED_LABEL, COMMENTED_BY_LABEL_PREFIX, HAS_CONFLICTS_LABEL_STR, + HOLD_LABEL_STR, LGTM_BY_LABEL_PREFIX, NEEDS_REBASE_LABEL_STR, TOX_STR, @@ -406,12 +407,15 @@ def test_prepare_retest_welcome_comment(self, pull_request_handler: PullRequestH 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.""" - pull_request_handler.github_webhook.enabled_labels = {"hold", "verified"} + # 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 @@ -419,7 +423,8 @@ def test_prepare_no_blockers_requirement_wip_disabled(self, pull_request_handler def test_prepare_no_blockers_requirement_hold_disabled(self, pull_request_handler: PullRequestHandler) -> None: """Test no blockers requirement when hold is disabled.""" - pull_request_handler.github_webhook.enabled_labels = {"wip", "verified"} + # 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 @@ -427,7 +432,10 @@ def test_prepare_no_blockers_requirement_hold_disabled(self, pull_request_handle 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.""" - pull_request_handler.github_webhook.enabled_labels = {"verified"} + # 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 diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 52c75d0e1..9e2885fa5 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" @@ -51,7 +54,19 @@ AUTOMERGE_LABEL_STR: "0E8A16", } -# Mapping from label strings to their configuration category names +# 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", @@ -91,7 +106,8 @@ # Default label colors - uses ALL_LABELS_DICT as the source of truth # These are used when no custom colors are configured via labels.colors -DEFAULT_LABEL_COLORS: dict[str, str] = ALL_LABELS_DICT +# Using MappingProxyType to prevent accidental mutation of the shared dict +DEFAULT_LABEL_COLORS: Mapping[str, str] = types.MappingProxyType(ALL_LABELS_DICT) # All configurable label categories (for enabled-labels config) # Note: reviewed-by is NOT in this list because it cannot be disabled From 852bb0b03a62a419522cce03f8f70a981d96826d Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 18:18:40 +0200 Subject: [PATCH 15/19] fix: address CodeRabbit review comments and add SafeRotatingFileHandler - Make PR ref fetch fatal instead of warning (fail-fast) - Sanitize enabled-labels logging to avoid large YAML blobs - Make git command test assertions less brittle (refspec matching) - Protect lgtm/approve labels from being silently disabled - Make ALL_LABELS_DICT immutable with MappingProxyType - Fix AsyncMock patching in test_labels_handler.py - Tighten Any types to Callable/Awaitable in test fixtures - Wrap PyGithub property access in asyncio.to_thread - Fix Headers type issues in tests - Remove assert statements from production code (use type: ignore) - Remove defensive isinstance check per CLAUDE.md - Add SafeRotatingFileHandler to fix log rotation crash --- webhook_server/libs/github_api.py | 41 ++- .../libs/handlers/labels_handler.py | 14 +- webhook_server/tests/test_github_api.py | 291 +++++++++++++----- webhook_server/tests/test_labels_handler.py | 38 ++- .../tests/test_safe_rotating_handler.py | 190 ++++++++++++ webhook_server/utils/constants.py | 5 +- webhook_server/utils/helpers.py | 6 + webhook_server/utils/safe_rotating_handler.py | 65 ++++ 8 files changed, 544 insertions(+), 106 deletions(-) create mode 100644 webhook_server/tests/test_safe_rotating_handler.py create mode 100644 webhook_server/utils/safe_rotating_handler.py diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 8d3aba5d7..811ed6dd1 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -291,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 --depth 1 {clone_url_with_token} {self.clone_repo_dir}", @@ -310,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, ) @@ -319,7 +321,7 @@ 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, ) @@ -342,18 +344,19 @@ def redact_output(value: str) -> str: # Fetch only this specific PR's ref pr_number = await asyncio.to_thread(lambda: pull_request.number) - rc, _, _ = await run_command( + 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: - self.logger.warning(f"{self.log_prefix} Failed to fetch PR {pr_number} ref") + 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 - assert checkout_ref is not None # mypy type narrowing - tag_name = checkout_ref.replace("refs/tags/", "") + 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}", @@ -369,8 +372,7 @@ def redact_output(value: str) -> str: else: # 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/", "") + checkout_target = checkout_ref.replace("refs/tags/", "") # type: ignore[union-attr] # Checkout target branch/tag rc, _, err = await run_command( @@ -697,8 +699,27 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: 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: {dropped}") + 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: diff --git a/webhook_server/libs/handlers/labels_handler.py b/webhook_server/libs/handlers/labels_handler.py index 09090107c..8e520cb74 100644 --- a/webhook_server/libs/handlers/labels_handler.py +++ b/webhook_server/libs/handlers/labels_handler.py @@ -65,6 +65,8 @@ def is_label_enabled(self, label: str) -> bool: - 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 = ( @@ -76,20 +78,16 @@ def is_label_enabled(self, label: str) -> bool: 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 - # Validate enabled_labels is a set (could be misconfigured) - if not isinstance(enabled_labels, set): - self.logger.warning( - f"{self.log_prefix} enabled_labels is not a set (got {type(enabled_labels).__name__}), " - "treating as all labels enabled" - ) - return True - # Check static labels using centralized category map if label in LABEL_CATEGORY_MAP: return LABEL_CATEGORY_MAP[label] in enabled_labels diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index a6514da83..17ae6b456 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 @@ -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 @@ -736,15 +736,16 @@ 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, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + ) -> None: """Test processing check run event.""" 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 +813,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 +843,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.""" @@ -871,7 +872,7 @@ async def test_get_pull_request_github_exception( @pytest.mark.asyncio async def test_get_pull_request_by_commit_with_pulls( - 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 commit with pulls.""" commit_data = { @@ -907,7 +908,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 +934,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 +963,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 +993,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 +1018,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 +1054,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 +1085,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 +1122,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 +1136,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 +1170,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 +1199,7 @@ 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, **_kwargs: object) -> tuple[bool, str, str]: 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,7 +1251,7 @@ 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]: + async def mock_run_command(**kwargs: object) -> tuple[bool, str, str]: command = kwargs.get("command", "") if "checkout main" in command: return (False, "", "Branch not found") @@ -1267,10 +1268,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,15 +1303,13 @@ 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]: + # Mock run_command: succeed for clone/checkout, fail for config commands only + async def mock_run_command(**kwargs: object) -> tuple[bool, str, str]: command = kwargs.get("command", "") if "config user.name" in command or "config user.email" in command: return (False, "", "Config failed") - # Targeted PR fetch (replaced git remote update) - if "fetch origin +refs/pull/" in command: - return (False, "", "Fetch failed") return (True, "", "") mock_logger = Mock() @@ -1327,15 +1326,81 @@ 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) == 3 # user.name, user.email, PR fetch + 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(**kwargs: object) -> tuple[bool, str, str]: + command = kwargs.get("command", "") + 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 = [call for call in 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: @@ -1362,7 +1427,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 ( @@ -1373,7 +1438,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: @@ -1402,7 +1467,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: @@ -1444,10 +1509,10 @@ async def test_clone_repository_empty_checkout_ref( async def test_clone_repository_checkout_ref_fetch_path_for_tag( 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 _clone_repository with checkout_ref fetches and checks out the correct tag. @@ -1484,7 +1549,7 @@ async def test_clone_repository_checkout_ref_fetch_path_for_tag( # Track commands executed executed_commands: list[str] = [] - async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str]: + async def mock_run_command(command: str, **_kwargs: object) -> tuple[bool, str, str]: executed_commands.append(command) return (True, "", "") @@ -1500,18 +1565,19 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str # Verify clone succeeded assert gh._repo_cloned is True - # Find the fetch command for the tag (uses explicit refspec format) - fetch_commands = [ - cmd - for cmd in executed_commands - if "fetch origin refs/tags/v1.0.0:refs/tags/v1.0.0" in cmd - ] + # 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 for tag v1.0.0, got: {fetch_commands}" + f"Expected exactly one fetch command containing refspec '{tag_refspec}', " + f"got: {fetch_commands}" ) - # Find the checkout command (tag name without refs/tags/ prefix) - checkout_commands = [cmd for cmd in executed_commands if "checkout v1.0.0" in cmd] + # 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}" ) @@ -1520,10 +1586,10 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str async def test_clone_repository_fetches_base_branch_for_pr( 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 _clone_repository fetches base branch before PR ref when pull_request is provided. @@ -1566,7 +1632,7 @@ async def test_clone_repository_fetches_base_branch_for_pr( # Track commands executed in order executed_commands: list[str] = [] - async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str]: + async def mock_run_command(command: str, **_kwargs: object) -> tuple[bool, str, str]: executed_commands.append(command) return (True, "", "") @@ -1582,34 +1648,35 @@ async def mock_run_command(command: str, **_kwargs: Any) -> tuple[bool, str, str # Verify clone succeeded assert gh._repo_cloned is True - # Find the base branch fetch command + # 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 origin release-1.0" in cmd + 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 release-1.0, " + f"Expected exactly one fetch command for base branch '{base_branch}', " f"got: {base_fetch_commands}" ) - # Find the PR ref fetch command - pr_fetch_commands = [ - cmd - for cmd in executed_commands - if "fetch origin +refs/pull/123/head:refs/remotes/origin/pr/123" in cmd - ] + # 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, got: {pr_fetch_commands}" + 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 origin release-1.0" in cmd - ) - pr_fetch_index = next( i for i, cmd in enumerate(executed_commands) - if "fetch origin +refs/pull/123/head" in cmd + 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}). " @@ -1766,7 +1833,11 @@ async def test_process_push_event_branch_push_skips_clone( @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, + logger: Mock, + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method removes temporary directory.""" mock_logger = Mock() @@ -1795,7 +1866,11 @@ 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, + logger: Mock, + to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method handles exceptions.""" mock_logger = Mock() @@ -1823,3 +1898,61 @@ 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() + # Get all warning calls and check the first one (non-string entries warning) + warning_calls = [call[0][0] for call in mock_logger.warning.call_args_list] + non_string_warning = warning_calls[0] + assert "Non-string entries in enabled-labels were ignored" in non_string_warning + 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_labels_handler.py b/webhook_server/tests/test_labels_handler.py index af5e0950e..8f0096575 100644 --- a/webhook_server/tests/test_labels_handler.py +++ b/webhook_server/tests/test_labels_handler.py @@ -314,8 +314,12 @@ async def test_add_label_static_label_wait_exception( 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")): + 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) @@ -549,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() @@ -585,7 +589,7 @@ async def test_label_by_user_comment_remove(self, labels_handler: LabelsHandler, 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", new_callable=AsyncMock, return_value=None): - with patch.object(labels_handler, "_add_label") as mock_add: + 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() @@ -608,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") ): @@ -643,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, @@ -1237,6 +1243,24 @@ def test_empty_enabled_labels_disables_all_except_reviewed_by(self, labels_handl 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} 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..d3f3150dc --- /dev/null +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -0,0 +1,190 @@ +"""Tests for SafeRotatingFileHandler.""" + +from __future__ import annotations + +import logging +import os +import tempfile +from unittest.mock import patch + +import simple_logger.logger + +from webhook_server.utils import helpers # noqa: F401 - import triggers patching +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(f"No such file: {path}") + 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(f"No such file: {src}") + 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 and immediately delete 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() + + # Delete the file before rollover + os.remove(log_file) + + # This should not crash + handler.doRollover() + + # Verify handler created a new file + 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/constants.py b/webhook_server/utils/constants.py index 9e2885fa5..f40c77f0a 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -102,12 +102,13 @@ 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] = types.MappingProxyType(ALL_LABELS_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 diff --git a/webhook_server/utils/helpers.py b/webhook_server/utils/helpers.py index 9d4dcede6..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( diff --git a/webhook_server/utils/safe_rotating_handler.py b/webhook_server/utils/safe_rotating_handler.py new file mode 100644 index 000000000..25d9d3a33 --- /dev/null +++ b/webhook_server/utils/safe_rotating_handler.py @@ -0,0 +1,65 @@ +"""Safe rotating file handler that handles missing backup files gracefully.""" + +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 FileNotFoundError during doRollover and continues + gracefully, ensuring logging is not interrupted by missing backup files. + """ + + def doRollover(self) -> None: # noqa: N802 + """Perform log file rollover with graceful handling of missing files. + + Catches FileNotFoundError that can occur when backup files are missing + and logs a warning instead of crashing. + """ + 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: + # Other OS errors (permissions, etc.) - continue gracefully + pass + + dfn = self.rotation_filename(f"{self.baseFilename}.1") + try: + if os.path.exists(dfn): + os.remove(dfn) + except FileNotFoundError: + pass + except OSError: + pass + + try: + self.rotate(self.baseFilename, dfn) + except FileNotFoundError: + # Base file was deleted - just create a new one + pass + except OSError: + pass + + if not self.delay: + self.stream = self._open() From fb242c3d2e798eec7a4af12cf0a5f45dd47df15a Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 19:58:38 +0200 Subject: [PATCH 16/19] fix: address CodeRabbit review - cleanup unused fixtures and noqa directives - Remove unused fixture parameters from test_process_check_run_event - Simplify list comprehension in test_clone_repository_pr_ref_fetch_failure_raises - Remove unused noqa directives from safe_rotating_handler.py and test file - Remove unused logger fixture params in cleanup tests --- webhook_server/tests/test_github_api.py | 9 +++------ webhook_server/tests/test_safe_rotating_handler.py | 4 +++- webhook_server/utils/safe_rotating_handler.py | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 17ae6b456..9a5e61d2d 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -736,10 +736,9 @@ 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: Headers, 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"}, @@ -1391,7 +1390,7 @@ async def mock_run_command(**kwargs: object) -> tuple[bool, str, str]: assert "Failed to fetch PR" in str(exc_info.value) # Verify error was logged - error_calls = [call for call in mock_logger.error.call_args_list] + error_calls = list(mock_logger.error.call_args_list) assert len(error_calls) >= 1 @pytest.mark.asyncio @@ -1836,7 +1835,6 @@ async def test_cleanup( self, minimal_hook_data: dict, minimal_headers: Headers, - logger: Mock, to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method removes temporary directory.""" @@ -1869,7 +1867,6 @@ async def test_cleanup_exception( self, minimal_hook_data: dict, minimal_headers: Headers, - logger: Mock, to_thread_sync: Callable[..., Awaitable[object]], ) -> None: """Test cleanup method handles exceptions.""" diff --git a/webhook_server/tests/test_safe_rotating_handler.py b/webhook_server/tests/test_safe_rotating_handler.py index d3f3150dc..e8c167f40 100644 --- a/webhook_server/tests/test_safe_rotating_handler.py +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -9,9 +9,11 @@ import simple_logger.logger -from webhook_server.utils import helpers # noqa: F401 - import triggers patching +from webhook_server.utils import helpers # Imported for side effect: patches simple_logger from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler +_ = helpers # Prevent unused import removal + class TestSafeRotatingFileHandler: """Tests for SafeRotatingFileHandler crash resilience.""" diff --git a/webhook_server/utils/safe_rotating_handler.py b/webhook_server/utils/safe_rotating_handler.py index 25d9d3a33..d1cda7130 100644 --- a/webhook_server/utils/safe_rotating_handler.py +++ b/webhook_server/utils/safe_rotating_handler.py @@ -17,7 +17,7 @@ class SafeRotatingFileHandler(RotatingFileHandler): gracefully, ensuring logging is not interrupted by missing backup files. """ - def doRollover(self) -> None: # noqa: N802 + def doRollover(self) -> None: """Perform log file rollover with graceful handling of missing files. Catches FileNotFoundError that can occur when backup files are missing From 1f480b069d3e47f6915a603c81c57de19becc1a2 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 20:19:40 +0200 Subject: [PATCH 17/19] fix: address CodeRabbit review comments - Fix ruff ARG002 unused fixture parameter - Fix order-dependent warning assertion - Fix unused import workaround with alias pattern - Fix ruff TRY003 by removing custom exception messages - Fix Windows portability issue with os.path.exists patch - Update doRollover docstring to reflect actual behavior - Add comprehensive OSError suppression documentation - Normalize mock signatures to match run_command --- webhook_server/tests/test_github_api.py | 25 ++++++--- .../tests/test_safe_rotating_handler.py | 27 +++++---- webhook_server/utils/safe_rotating_handler.py | 55 ++++++++++++++++--- 3 files changed, 82 insertions(+), 25 deletions(-) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 9a5e61d2d..b3c8bbe2e 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1198,7 +1198,9 @@ 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: object) -> tuple[bool, str, str]: + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: if "git clone" in command: return (False, "", "Permission denied") return (True, "", "") @@ -1250,8 +1252,9 @@ 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: object) -> tuple[bool, str, str]: - command = kwargs.get("command", "") + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: if "checkout main" in command: return (False, "", "Branch not found") return (True, "", "") @@ -1305,8 +1308,9 @@ async def test_clone_repository_git_config_warnings( mock_pr.number = 123 # Mock run_command: succeed for clone/checkout, fail for config commands only - async def mock_run_command(**kwargs: object) -> tuple[bool, str, str]: - command = kwargs.get("command", "") + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: if "config user.name" in command or "config user.email" in command: return (False, "", "Config failed") return (True, "", "") @@ -1942,10 +1946,15 @@ def get_value_side_effect(value: str, *_args: object, **kwargs: object) -> objec # Verify warning was logged about non-string entries mock_logger.warning.assert_called() - # Get all warning calls and check the first one (non-string entries warning) + # 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 = warning_calls[0] - assert "Non-string entries in enabled-labels were ignored" in non_string_warning + 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 diff --git a/webhook_server/tests/test_safe_rotating_handler.py b/webhook_server/tests/test_safe_rotating_handler.py index e8c167f40..f48a4e2bb 100644 --- a/webhook_server/tests/test_safe_rotating_handler.py +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -9,11 +9,9 @@ import simple_logger.logger -from webhook_server.utils import helpers # Imported for side effect: patches simple_logger +from webhook_server.utils import helpers as _ # noqa: F401 # Imported for side effect: patches simple_logger from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler -_ = helpers # Prevent unused import removal - class TestSafeRotatingFileHandler: """Tests for SafeRotatingFileHandler crash resilience.""" @@ -94,7 +92,7 @@ def mock_exists(path: str) -> bool: def mock_remove(path: str) -> None: if path.endswith(".1"): - raise FileNotFoundError(f"No such file: {path}") + raise FileNotFoundError() return original_remove(path) with patch("os.path.exists", side_effect=mock_exists): @@ -133,7 +131,7 @@ def mock_exists(path: str) -> bool: def mock_rename(src: str, dst: str) -> None: if ".1" in src or ".2" in src: - raise FileNotFoundError(f"No such file: {src}") + raise FileNotFoundError() return original_rename(src, dst) with patch("os.path.exists", side_effect=mock_exists): @@ -157,7 +155,7 @@ def test_rollover_with_nonexistent_base_file(self) -> None: backupCount=3, ) try: - # Create and immediately delete the log file + # Create the log file with open(log_file, "w") as f: f.write("initial content") @@ -165,11 +163,20 @@ def test_rollover_with_nonexistent_base_file(self) -> None: if handler.stream is None: handler.stream = handler._open() - # Delete the file before rollover - os.remove(log_file) + # 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 - # This should not crash - handler.doRollover() + 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 diff --git a/webhook_server/utils/safe_rotating_handler.py b/webhook_server/utils/safe_rotating_handler.py index d1cda7130..88bf9fa43 100644 --- a/webhook_server/utils/safe_rotating_handler.py +++ b/webhook_server/utils/safe_rotating_handler.py @@ -1,4 +1,27 @@ -"""Safe rotating file handler that handles missing backup files gracefully.""" +"""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 @@ -13,15 +36,27 @@ class SafeRotatingFileHandler(RotatingFileHandler): backup log files are deleted externally (e.g., by logrotate, manual cleanup, or disk space management) during the rollover process. - This implementation catches FileNotFoundError during doRollover and continues - gracefully, ensuring logging is not interrupted by missing backup files. + 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 missing files. + """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. - Catches FileNotFoundError that can occur when backup files are missing - and logs a warning instead of crashing. + 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() @@ -41,7 +76,8 @@ def doRollover(self) -> None: # File was deleted between exists check and operation - ignore pass except OSError: - # Other OS errors (permissions, etc.) - continue gracefully + # Broad suppression intentional: logging must never crash. + # See module docstring for full rationale. pass dfn = self.rotation_filename(f"{self.baseFilename}.1") @@ -49,8 +85,11 @@ def doRollover(self) -> None: 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: @@ -59,6 +98,8 @@ def doRollover(self) -> None: # 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: From 1baa0ab82a18660327e8bdb67e00d305c322f939 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 20:33:42 +0200 Subject: [PATCH 18/19] fix: address CodeRabbit review comments - Fix mock_run_command signature to capture positional args - Remove unused noqa: F401 directive - Add OSError protection to _open() call in doRollover - Remove unused minimal_hook_data fixture parameter --- webhook_server/tests/test_github_api.py | 10 +++--- .../tests/test_safe_rotating_handler.py | 35 ++++++++++++++++++- webhook_server/utils/safe_rotating_handler.py | 7 +++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index b3c8bbe2e..3bc09be53 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -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: Headers, 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"}, @@ -1373,8 +1371,10 @@ async def test_clone_repository_pr_ref_fetch_failure_raises( mock_pr.number = 456 # Mock run_command: succeed for clone, fail for PR ref fetch - async def mock_run_command(**kwargs: object) -> tuple[bool, str, str]: - command = kwargs.get("command", "") + async def mock_run_command( + command: str, log_prefix: str, **_kwargs: object + ) -> tuple[bool, str, str]: + _ = log_prefix # Unused parameter if "fetch origin +refs/pull/" in command: return (False, "", "Fetch failed: PR ref not found") return (True, "", "") diff --git a/webhook_server/tests/test_safe_rotating_handler.py b/webhook_server/tests/test_safe_rotating_handler.py index f48a4e2bb..6d7b672d2 100644 --- a/webhook_server/tests/test_safe_rotating_handler.py +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -9,7 +9,6 @@ import simple_logger.logger -from webhook_server.utils import helpers as _ # noqa: F401 # Imported for side effect: patches simple_logger from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler @@ -184,6 +183,40 @@ def mock_exists(path: str) -> bool: 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.""" diff --git a/webhook_server/utils/safe_rotating_handler.py b/webhook_server/utils/safe_rotating_handler.py index 88bf9fa43..983e9d7e8 100644 --- a/webhook_server/utils/safe_rotating_handler.py +++ b/webhook_server/utils/safe_rotating_handler.py @@ -103,4 +103,9 @@ def doRollover(self) -> None: pass if not self.delay: - self.stream = self._open() + 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 From 546857cd55e6152a155b7d26bf816cceaaa6a6ac Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 13 Jan 2026 22:10:55 +0200 Subject: [PATCH 19/19] fix: address CodeRabbit review and revert shallow clone - Fix mock_run_command parameter handling for log_prefix - Add explicit helpers import for test isolation - Remove redundant Headers() wrapping - Revert shallow clone to fix merge unrelated histories bug --- webhook_server/libs/github_api.py | 2 +- webhook_server/tests/test_github_api.py | 7 +++++-- webhook_server/tests/test_safe_rotating_handler.py | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 811ed6dd1..1faefe3b4 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -295,7 +295,7 @@ async def _clone_repository( clone_url_with_token = clone_url.replace("https://", f"https://{github_token}@") rc, _, err = await run_command( - command=f"git clone --depth 1 {clone_url_with_token} {self.clone_repo_dir}", + command=f"git clone {clone_url_with_token} {self.clone_repo_dir}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.mask_sensitive, diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 3bc09be53..e3f6c1581 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1199,6 +1199,7 @@ async def test_clone_repository_clone_failure( 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, "", "") @@ -1253,6 +1254,7 @@ async def test_clone_repository_checkout_failure( 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, "", "") @@ -1309,6 +1311,7 @@ async def test_clone_repository_git_config_warnings( 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") return (True, "", "") @@ -1374,7 +1377,7 @@ async def test_clone_repository_pr_ref_fetch_failure_raises( async def mock_run_command( command: str, log_prefix: str, **_kwargs: object ) -> tuple[bool, str, str]: - _ = log_prefix # Unused parameter + del log_prefix # unused if "fetch origin +refs/pull/" in command: return (False, "", "Fetch failed: PR ref not found") return (True, "", "") @@ -1500,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, ) diff --git a/webhook_server/tests/test_safe_rotating_handler.py b/webhook_server/tests/test_safe_rotating_handler.py index 6d7b672d2..1bfb924c1 100644 --- a/webhook_server/tests/test_safe_rotating_handler.py +++ b/webhook_server/tests/test_safe_rotating_handler.py @@ -9,6 +9,7 @@ import simple_logger.logger +import webhook_server.utils.helpers # noqa: F401 from webhook_server.utils.safe_rotating_handler import SafeRotatingFileHandler