From 309cf67652f31ce1277e16903fa9a91074c81e46 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 1 Jul 2026 16:14:03 +0300 Subject: [PATCH 01/10] feat: support custom additional information in PR welcome message Add welcome-extra-info config option (global + per-repo) and .github-webhook-server-welcome-message.md file-based override to inject custom markdown into PR welcome messages. Priority chain (first match wins): 1. .github-webhook-server-welcome-message.md file in repo 2. .github-webhook-server.yaml welcome-extra-info 3. config.yaml per-repo welcome-extra-info 4. config.yaml global welcome-extra-info Empty file or empty string in config explicitly suppresses inherited values. Content is injected as-is (markdown) in a new Additional Information section after Tips. Closes: #1143 Assisted-by: Claude Signed-off-by: rnetser --- examples/config.yaml | 6 + webhook_server/config/schema.yaml | 14 +++ webhook_server/libs/github_api.py | 62 ++++++++++ .../libs/handlers/pull_request_handler.py | 14 +++ webhook_server/tests/test_github_api.py | 112 +++++++++++++++++- .../tests/test_pull_request_handler.py | 99 ++++++++++++++++ 6 files changed, 305 insertions(+), 2 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index cd77362f0..23373f90f 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -35,6 +35,12 @@ create-issue-for-new-pr: true # Global default: create tracking issues for new P cherry-pick-assign-to-pr-author: true # Default: true - assign cherry-pick PRs to the original PR author +# Additional information to display at the end of PR welcome messages (markdown) +# welcome-extra-info: | +# **Note:** Please review the contribution guide before merging. +# - Ensure tests pass +# - Update documentation if needed + # Commands allowed on draft PRs (optional) # If not set: commands are blocked on draft PRs (default behavior) # If empty list []: all commands allowed on draft PRs diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 815b99545..ffad8f448 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -251,6 +251,13 @@ properties: additionalProperties: false ai-features: $ref: '#/$defs/ai-features' + welcome-extra-info: + type: string + maxLength: 10240 + description: | + Additional information to display at the end of the PR welcome message (global default). + Content is injected as-is (markdown). Can be overridden per-repo. + An empty string explicitly clears any inherited value. security-checks: $ref: '#/$defs/security-checks' labels: @@ -523,6 +530,13 @@ properties: items: type: string description: Required labels for PR to be marked as can-be-merged + welcome-extra-info: + type: string + maxLength: 10240 + description: | + Additional information to display at the end of the PR welcome message (per-repo override). + Content is injected as-is (markdown). Overrides the global setting. + An empty string explicitly clears any inherited value. conventional-title: type: string description: | diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index deef20fa4..b2e602939 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -17,6 +17,7 @@ import httpx from github import GithubException from github.Commit import Commit +from github.GithubException import UnknownObjectException from github.PullRequest import PullRequest from github.Repository import Repository from starlette.datastructures import Headers @@ -142,6 +143,7 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. self.github_api: github.Github | None = None self.initial_rate_limit_remaining: int | None = None self.requester_wrapper: CountingRequester | None = None + self.welcome_extra_info: str = "" if not self.config.repository_data: raise RepositoryNotFoundInConfigError(f"Repository {self.repository_name} not found in config file") @@ -689,6 +691,12 @@ async def process(self) -> Any: github_api_call(lambda: pull_request.head.sha, logger=self.logger, log_prefix=self.log_prefix), ) + if self.github_event == "issue_comment" or ( + self.github_event == "pull_request" + and self.hook_data.get("action") in ("opened", "reopened", "ready_for_review") + ): + await self._load_welcome_extra_info_from_file() + # Clone repository for local file processing (OWNERS, changed files) # For check_run, status, and pull_request_review_thread events, # cloning happens later only when needed (inside their respective handlers) @@ -1109,6 +1117,10 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) + self.welcome_extra_info = self.config.get_value( + "welcome-extra-info", return_on_none="", extra_dict=repository_config + ) + async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: """Add dynamic entries to trusted-committers list. @@ -1142,6 +1154,56 @@ async def _build_trusted_committers(self, api_users: list[str | None] | None = N self.logger.debug(f"{self.log_prefix} Trusted committers: {self.security_trusted_committers}") + async def _load_welcome_extra_info_from_file(self) -> None: + """Load welcome extra info from .github-webhook-server-welcome-message.md in the repo. + + This file takes priority over all config-based welcome-extra-info settings. + File must be UTF-8 and at most 10KB. + """ + try: + _path = await github_api_call( + lambda: self.repository.get_contents(".github-webhook-server-welcome-message.md"), + logger=self.logger, + log_prefix=self.log_prefix, + ) + content_file = _path[0] if isinstance(_path, list) else _path + + file_size = await github_api_call(lambda: content_file.size, logger=self.logger, log_prefix=self.log_prefix) + if file_size > 10240: + self.logger.warning( + f"{self.log_prefix} .github-webhook-server-welcome-message.md is too large " + f"({file_size} bytes, max 10240). Skipping file, using config value." + ) + return + + raw_content = await github_api_call( + lambda: content_file.decoded_content, logger=self.logger, log_prefix=self.log_prefix + ) + decoded = raw_content.decode("utf-8").strip() + self.welcome_extra_info = decoded + if decoded: + self.logger.info( + f"{self.log_prefix} Loaded welcome extra info from .github-webhook-server-welcome-message.md " + f"({len(decoded)} chars)" + ) + else: + self.logger.info( + f"{self.log_prefix} Empty .github-webhook-server-welcome-message.md found, " + "suppressing config-based welcome extra info" + ) + except UnknownObjectException: + self.logger.debug( + f"{self.log_prefix} .github-webhook-server-welcome-message.md not found, using config value" + ) + except UnicodeDecodeError: + self.logger.warning( + f"{self.log_prefix} .github-webhook-server-welcome-message.md is not valid UTF-8, skipping file" + ) + except Exception: + self.logger.exception( + f"{self.log_prefix} Error loading .github-webhook-server-welcome-message.md, using config value" + ) + async def get_pull_request(self, number: int | None = None) -> PullRequest | None: if number: self.logger.debug(f"{self.log_prefix} Attempting to get PR by number: {number}") diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index a511123a5..7f641da40 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -527,6 +527,7 @@ def _prepare_welcome_comment(self) -> str: ### 💡 Tips {self._prepare_tips_section} +{self._prepare_extra_info_welcome_section}\ For more information, please refer to the project documentation or contact the maintainers. """ @@ -691,6 +692,19 @@ def _prepare_tips_section(self) -> str: return "\n".join(tips) + @property + def _prepare_extra_info_welcome_section(self) -> str: + """Prepare the Additional Information section for the welcome comment. + + Renders user-provided extra information from configuration or + .github-webhook-server-welcome-message.md file. + Content is injected as-is (markdown). + """ + if not self.github_webhook.welcome_extra_info: + return "" + + return f"\n### 📌 Additional Information\n\n{self.github_webhook.welcome_extra_info}\n" + @property def _prepare_ai_features_welcome_section(self) -> str: """Prepare the AI Features section for the welcome comment. diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index fecbe9258..56646d393 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -4,10 +4,10 @@ from collections.abc import Awaitable, Callable from pathlib import Path from typing import Any -from unittest.mock import AsyncMock, Mock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest -from github.GithubException import GithubException +from github.GithubException import GithubException, UnknownObjectException from simple_logger.logger import get_logger from starlette.datastructures import Headers @@ -2812,3 +2812,111 @@ def mock_get_value(value: str, return_on_none: Any = None, extra_dict: Any = Non "version": True, "title": True, } + + +class TestLoadWelcomeExtraInfoFromFile: + """Tests for _load_welcome_extra_info_from_file.""" + + @pytest.fixture() + def github_webhook(self, process_github_webhook): + """Create a GithubWebhook instance for welcome extra info tests.""" + gw = process_github_webhook + gw.welcome_extra_info = "config value" + gw.repository = MagicMock() + return gw + + @pytest.mark.asyncio() + async def test_load_from_file_success(self, github_webhook): + content_file = MagicMock() + content_file.size = 100 + content_file.decoded_content = b"# Custom welcome\nSome info" + + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.return_value = content_file + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "# Custom welcome\nSome info" + + @pytest.mark.asyncio() + async def test_load_from_file_too_large(self, github_webhook): + content_file = MagicMock() + content_file.size = 20000 + + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.return_value = content_file + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "config value" + + @pytest.mark.asyncio() + async def test_load_from_file_not_found(self, github_webhook): + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.side_effect = UnknownObjectException(404, "Not found", None) + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "config value" + + @pytest.mark.asyncio() + async def test_load_from_file_exception(self, github_webhook): + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.side_effect = Exception("API error") + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "config value" + + @pytest.mark.asyncio() + async def test_load_from_file_empty(self, github_webhook): + content_file = MagicMock() + content_file.size = 0 + content_file.decoded_content = b"" + + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.return_value = content_file + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "" + + @pytest.mark.asyncio() + async def test_load_from_file_list_response(self, github_webhook): + content_file = MagicMock() + content_file.size = 50 + content_file.decoded_content = b"List content" + + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.return_value = [content_file] + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "List content" + + @pytest.mark.asyncio() + async def test_load_from_file_unicode_error(self, github_webhook): + content_file = MagicMock() + content_file.size = 10 + content_file.decoded_content = b"\xff\xfe invalid" + + async def mock_to_thread(func, *args, **kwargs): + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + github_webhook.repository.get_contents.return_value = content_file + await github_webhook._load_welcome_extra_info_from_file() + + assert github_webhook.welcome_extra_info == "config value" diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 4a7642a18..35f8cee09 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -98,6 +98,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.security_committer_identity_check = True mock_webhook.security_mandatory = True mock_webhook.last_committer = "test-user" + mock_webhook.welcome_extra_info = "" mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) return mock_webhook @@ -3295,3 +3296,101 @@ async def test_process_reopened_action_does_not_call_test_oracle( await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) mock_test_oracle.assert_not_called() mock_create_task.assert_not_called() + + +class TestPrepareExtraInfoWelcomeSection: + """Tests for _prepare_extra_info_welcome_section.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock(spec=GithubWebhook) + mock_webhook.hook_data = { + "action": "opened", + "pull_request": {"number": 123, "merged": False, "title": "Test PR"}, + "sender": {"login": "test-user"}, + "label": {"name": "bug"}, + } + mock_webhook.logger = MagicMock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository_full_name = "test-org/test-repo" + mock_webhook.repository = Mock() + mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" + mock_webhook.parent_committer = "test-user" + mock_webhook.auto_verified_and_merged_users = ["test-user"] + mock_webhook.create_issue_for_new_pr = True + mock_webhook.verified_job = True + mock_webhook.build_and_push_container = True + mock_webhook.container_repository_and_tag = Mock(return_value="test-repo:pr-123") + mock_webhook.can_be_merged_required_labels = [] + mock_webhook.set_auto_merge_prs = [] + mock_webhook.auto_merge_enabled = True + mock_webhook.container_repository = "docker.io/org/repo" + mock_webhook.conventional_title = False + mock_webhook.minimum_lgtm = 1 + mock_webhook.container_repository_username = "test-user" + mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret + mock_webhook.github_api = Mock() + mock_webhook.tox = True + mock_webhook.pre_commit = True + mock_webhook.python_module_install = False + mock_webhook.pypi = False + mock_webhook.token = TEST_GITHUB_TOKEN + mock_webhook.auto_verify_cherry_picked_prs = True + mock_webhook.cherry_pick_assign_to_pr_author = True + mock_webhook.last_commit = Mock() + mock_webhook.ctx = None + mock_webhook.enabled_labels = None + mock_webhook.custom_check_runs = [] + mock_webhook.ai_features = None + mock_webhook.required_conversation_resolution = False + mock_webhook.security_suspicious_paths = [] + mock_webhook.security_committer_identity_check = True + mock_webhook.security_mandatory = True + mock_webhook.last_committer = "test-user" + mock_webhook.welcome_extra_info = "" + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + return mock_webhook + + @pytest.fixture + def mock_owners_file_handler(self) -> Mock: + """Create a mock OwnersFileHandler instance.""" + mock_handler = Mock(spec=OwnersFileHandler) + mock_handler.all_pull_request_approvers = ["approver1", "approver2"] + mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] + mock_handler.root_approvers = ["root-approver"] + mock_handler.root_reviewers = ["root-reviewer"] + mock_handler.assign_reviewers = AsyncMock() + return mock_handler + + @pytest.fixture + def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> PullRequestHandler: + """Create a PullRequestHandler instance with mocked dependencies.""" + handler = PullRequestHandler(mock_github_webhook, mock_owners_file_handler) + handler.labels_handler = Mock() + handler.labels_handler.is_label_enabled = Mock(return_value=True) + return handler + + def test_extra_info_present(self, pull_request_handler): + pull_request_handler.github_webhook.welcome_extra_info = "Custom info text" + result = pull_request_handler._prepare_extra_info_welcome_section + assert "### \U0001f4cc Additional Information\n\n" in result + assert "Custom info text" in result + + def test_extra_info_empty(self, pull_request_handler): + pull_request_handler.github_webhook.welcome_extra_info = "" + result = pull_request_handler._prepare_extra_info_welcome_section + assert result == "" + + def test_extra_info_none(self, pull_request_handler): + pull_request_handler.github_webhook.welcome_extra_info = None + result = pull_request_handler._prepare_extra_info_welcome_section + assert result == "" + + def test_extra_info_multiline_markdown(self, pull_request_handler): + pull_request_handler.github_webhook.welcome_extra_info = "**Bold** and *italic*\n- Item 1\n- Item 2" + result = pull_request_handler._prepare_extra_info_welcome_section + assert "### \U0001f4cc Additional Information\n\n" in result + assert "**Bold** and *italic*" in result + assert "- Item 1" in result From 4a8d4091427c2ae95b36d67ef2f6b6909512db0a Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 1 Jul 2026 20:53:39 +0300 Subject: [PATCH 02/10] fix: address Qodo review findings - Add type hints to all new test functions/fixtures - Add runtime 10KB size guard for config-loaded welcome-extra-info - Move file loading from process() to regenerate_welcome_message() - Add 4 schema validation tests for welcome-extra-info Signed-off-by: rnetser Assisted-by: Claude Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 18 ++++- .../libs/handlers/pull_request_handler.py | 1 + webhook_server/tests/test_config_schema.py | 69 +++++++++++++++++++ webhook_server/tests/test_github_api.py | 56 ++++++++++++--- .../tests/test_pull_request_handler.py | 8 +-- 5 files changed, 137 insertions(+), 15 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index b2e602939..b416e36ff 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -691,9 +691,10 @@ async def process(self) -> Any: github_api_call(lambda: pull_request.head.sha, logger=self.logger, log_prefix=self.log_prefix), ) - if self.github_event == "issue_comment" or ( - self.github_event == "pull_request" - and self.hook_data.get("action") in ("opened", "reopened", "ready_for_review") + if self.github_event == "pull_request" and self.hook_data.get("action") in ( + "opened", + "reopened", + "ready_for_review", ): await self._load_welcome_extra_info_from_file() @@ -1121,6 +1122,17 @@ def _sanitize_item(item: Any) -> str: "welcome-extra-info", return_on_none="", extra_dict=repository_config ) + if not isinstance(self.welcome_extra_info, str): + self.logger.warning( + f"welcome-extra-info must be a string, got {type(self.welcome_extra_info).__name__}. Ignoring." + ) + self.welcome_extra_info = "" + elif len(self.welcome_extra_info) > 10240: + self.logger.warning( + f"welcome-extra-info exceeds 10KB limit ({len(self.welcome_extra_info)} bytes). Ignoring." + ) + self.welcome_extra_info = "" + async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: """Add dynamic entries to trusted-committers list. diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 7f641da40..19e037d98 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1934,6 +1934,7 @@ async def regenerate_welcome_message(self, pull_request: PullRequest) -> None: If a welcome message exists, it will be updated. If no welcome message exists, a new one will be created. """ + await self.github_webhook._load_welcome_extra_info_from_file() welcome_msg = self._prepare_welcome_comment() def find_and_update_welcome_comment() -> bool: diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 6a6002570..094cd810b 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -1006,3 +1006,72 @@ def test_ai_features_resolve_cherry_pick_conflicts_with_ai_repository_level( } finally: shutil.rmtree(temp_dir) + + def test_welcome_extra_info_global_valid( + self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test valid welcome-extra-info at global level.""" + config = valid_minimal_config.copy() + config["welcome-extra-info"] = "Please review guidelines." + + temp_dir = self.create_temp_config_dir_and_data(config) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config_obj = Config() + assert config_obj.root_data["welcome-extra-info"] == "Please review guidelines." + finally: + shutil.rmtree(temp_dir) + + def test_welcome_extra_info_global_type_violation( + self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test that welcome-extra-info with integer type still loads (runtime guard handles it).""" + config = valid_minimal_config.copy() + config["welcome-extra-info"] = 12345 + + temp_dir = self.create_temp_config_dir_and_data(config) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config_obj = Config() + assert config_obj.root_data["welcome-extra-info"] == 12345 + finally: + shutil.rmtree(temp_dir) + + def test_welcome_extra_info_repository_level( + self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test valid welcome-extra-info at per-repo level.""" + config = valid_minimal_config.copy() + config["repositories"]["test-repo"]["welcome-extra-info"] = "Repo-specific welcome info." + + temp_dir = self.create_temp_config_dir_and_data(config) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config_obj = Config() + repo_data = config_obj.root_data["repositories"]["test-repo"] + assert repo_data["welcome-extra-info"] == "Repo-specific welcome info." + finally: + shutil.rmtree(temp_dir) + + def test_welcome_extra_info_max_length( + self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test that welcome-extra-info exceeding 10240 chars loads (runtime guard handles it).""" + config = valid_minimal_config.copy() + config["welcome-extra-info"] = "x" * 10241 + + temp_dir = self.create_temp_config_dir_and_data(config) + + try: + monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + + config_obj = Config() + assert len(config_obj.root_data["welcome-extra-info"]) == 10241 + finally: + shutil.rmtree(temp_dir) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 56646d393..7969e5fd1 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2818,7 +2818,7 @@ class TestLoadWelcomeExtraInfoFromFile: """Tests for _load_welcome_extra_info_from_file.""" @pytest.fixture() - def github_webhook(self, process_github_webhook): + def github_webhook(self, process_github_webhook: GithubWebhook) -> GithubWebhook: """Create a GithubWebhook instance for welcome extra info tests.""" gw = process_github_webhook gw.welcome_extra_info = "config value" @@ -2826,7 +2826,7 @@ def github_webhook(self, process_github_webhook): return gw @pytest.mark.asyncio() - async def test_load_from_file_success(self, github_webhook): + async def test_load_from_file_success(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 100 content_file.decoded_content = b"# Custom welcome\nSome info" @@ -2841,7 +2841,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "# Custom welcome\nSome info" @pytest.mark.asyncio() - async def test_load_from_file_too_large(self, github_webhook): + async def test_load_from_file_too_large(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 20000 @@ -2855,7 +2855,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "config value" @pytest.mark.asyncio() - async def test_load_from_file_not_found(self, github_webhook): + async def test_load_from_file_not_found(self, github_webhook: GithubWebhook) -> None: async def mock_to_thread(func, *args, **kwargs): return func(*args, **kwargs) if callable(func) else func @@ -2866,7 +2866,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "config value" @pytest.mark.asyncio() - async def test_load_from_file_exception(self, github_webhook): + async def test_load_from_file_exception(self, github_webhook: GithubWebhook) -> None: async def mock_to_thread(func, *args, **kwargs): return func(*args, **kwargs) if callable(func) else func @@ -2877,7 +2877,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "config value" @pytest.mark.asyncio() - async def test_load_from_file_empty(self, github_webhook): + async def test_load_from_file_empty(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 0 content_file.decoded_content = b"" @@ -2892,7 +2892,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "" @pytest.mark.asyncio() - async def test_load_from_file_list_response(self, github_webhook): + async def test_load_from_file_list_response(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 50 content_file.decoded_content = b"List content" @@ -2907,7 +2907,7 @@ async def mock_to_thread(func, *args, **kwargs): assert github_webhook.welcome_extra_info == "List content" @pytest.mark.asyncio() - async def test_load_from_file_unicode_error(self, github_webhook): + async def test_load_from_file_unicode_error(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 10 content_file.decoded_content = b"\xff\xfe invalid" @@ -2920,3 +2920,43 @@ async def mock_to_thread(func, *args, **kwargs): await github_webhook._load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "config value" + + +class TestWelcomeExtraInfoConfigGuard: + """Tests for welcome-extra-info runtime guard in _repo_data_from_config.""" + + @pytest.fixture() + def github_webhook(self, process_github_webhook: GithubWebhook) -> GithubWebhook: + """Create a GithubWebhook instance for config guard tests.""" + return process_github_webhook + + def test_config_welcome_extra_info_oversized(self, github_webhook: GithubWebhook) -> None: + """Test that oversized config value is cleared by the guard.""" + oversized_value = "x" * 10241 + github_webhook.config.get_value = MagicMock( + side_effect=lambda value="", return_on_none=None, extra_dict=None: ( + oversized_value if value == "welcome-extra-info" else MagicMock() + ) + ) + github_webhook._repo_data_from_config(repository_config={}) + assert github_webhook.welcome_extra_info == "" + + def test_config_welcome_extra_info_not_string(self, github_webhook: GithubWebhook) -> None: + """Test that non-string config value is cleared by the guard.""" + github_webhook.config.get_value = MagicMock( + side_effect=lambda value="", return_on_none=None, extra_dict=None: ( + 12345 if value == "welcome-extra-info" else MagicMock() + ) + ) + github_webhook._repo_data_from_config(repository_config={}) + assert github_webhook.welcome_extra_info == "" + + def test_config_welcome_extra_info_valid(self, github_webhook: GithubWebhook) -> None: + """Test that valid config value is kept.""" + github_webhook.config.get_value = MagicMock( + side_effect=lambda value="", return_on_none=None, extra_dict=None: ( + "Valid info" if value == "welcome-extra-info" else MagicMock() + ) + ) + github_webhook._repo_data_from_config(repository_config={}) + assert github_webhook.welcome_extra_info == "Valid info" diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 35f8cee09..17fab1bd1 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -3372,23 +3372,23 @@ def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handl handler.labels_handler.is_label_enabled = Mock(return_value=True) return handler - def test_extra_info_present(self, pull_request_handler): + def test_extra_info_present(self, pull_request_handler: PullRequestHandler) -> None: pull_request_handler.github_webhook.welcome_extra_info = "Custom info text" result = pull_request_handler._prepare_extra_info_welcome_section assert "### \U0001f4cc Additional Information\n\n" in result assert "Custom info text" in result - def test_extra_info_empty(self, pull_request_handler): + def test_extra_info_empty(self, pull_request_handler: PullRequestHandler) -> None: pull_request_handler.github_webhook.welcome_extra_info = "" result = pull_request_handler._prepare_extra_info_welcome_section assert result == "" - def test_extra_info_none(self, pull_request_handler): + def test_extra_info_none(self, pull_request_handler: PullRequestHandler) -> None: pull_request_handler.github_webhook.welcome_extra_info = None result = pull_request_handler._prepare_extra_info_welcome_section assert result == "" - def test_extra_info_multiline_markdown(self, pull_request_handler): + def test_extra_info_multiline_markdown(self, pull_request_handler: PullRequestHandler) -> None: pull_request_handler.github_webhook.welcome_extra_info = "**Bold** and *italic*\n- Item 1\n- Item 2" result = pull_request_handler._prepare_extra_info_welcome_section assert "### \U0001f4cc Additional Information\n\n" in result From 4fa63453b853da82fde8229810c5bb89b48c8c7b Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 1 Jul 2026 21:39:14 +0300 Subject: [PATCH 03/10] fix: use byte-based size guard and verify schema definitions - Change welcome-extra-info size guard from len(str) to len(str.encode('utf-8')) - Replace runtime-behavior schema tests with schema-definition verification - Test byte-based enforcement with multibyte UTF-8 characters Signed-off-by: rnetser Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- AGENTS.md | 3 +- docs/container-and-pypi-workflows.md | 6 +-- docs/testing-and-maintenance.md | 1 + webhook_server/libs/github_api.py | 7 ++- webhook_server/tests/test_config_schema.py | 52 ++++++++++------------ webhook_server/tests/test_github_api.py | 5 ++- 6 files changed, 34 insertions(+), 40 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 994117c5d..d826c1201 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,7 +61,8 @@ pr.create_issue_comment("Comment") ### Anti-Defensive Programming: Fail-Fast ```python # ❌ WRONG — config is required, ALWAYS provided -if self.config: value = self.config.get_value("key") +if self.config: + value = self.config.get_value("key") # ✅ CORRECT — fail-fast; KeyError = legitimate bug value = self.config.get_value("key") diff --git a/docs/container-and-pypi-workflows.md b/docs/container-and-pypi-workflows.md index 30d386a35..6fdfb1c16 100644 --- a/docs/container-and-pypi-workflows.md +++ b/docs/container-and-pypi-workflows.md @@ -82,11 +82,7 @@ The tag-selection logic in the code is: ```python if is_merged: pull_request_branch = pull_request.base.ref - tag = ( - pull_request_branch - if pull_request_branch not in (OTHER_MAIN_BRANCH, "main") - else self.container_tag - ) + tag = pull_request_branch if pull_request_branch not in (OTHER_MAIN_BRANCH, "main") else self.container_tag else: tag = f"pr-{pull_request.number}" ``` diff --git a/docs/testing-and-maintenance.md b/docs/testing-and-maintenance.md index d7c33e3e6..93aa90eaf 100644 --- a/docs/testing-and-maintenance.md +++ b/docs/testing-and-maintenance.md @@ -75,6 +75,7 @@ A representative example from `webhook_server/tests/test_app.py` posts a webhook def client(self) -> TestClient: return TestClient(FASTAPI_APP) + @patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"}) @patch("webhook_server.app.GithubWebhook") def test_process_webhook_success( diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index b416e36ff..c040843e9 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -1127,10 +1127,9 @@ def _sanitize_item(item: Any) -> str: f"welcome-extra-info must be a string, got {type(self.welcome_extra_info).__name__}. Ignoring." ) self.welcome_extra_info = "" - elif len(self.welcome_extra_info) > 10240: - self.logger.warning( - f"welcome-extra-info exceeds 10KB limit ({len(self.welcome_extra_info)} bytes). Ignoring." - ) + elif len(self.welcome_extra_info.encode("utf-8")) > 10240: + _byte_len = len(self.welcome_extra_info.encode("utf-8")) + self.logger.warning(f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring.") self.welcome_extra_info = "" async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 094cd810b..cb28eaf13 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -1024,22 +1024,21 @@ def test_welcome_extra_info_global_valid( finally: shutil.rmtree(temp_dir) - def test_welcome_extra_info_global_type_violation( - self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch - ) -> None: - """Test that welcome-extra-info with integer type still loads (runtime guard handles it).""" - config = valid_minimal_config.copy() - config["welcome-extra-info"] = 12345 - - temp_dir = self.create_temp_config_dir_and_data(config) + def test_welcome_extra_info_schema_defines_string_type(self) -> None: + """Test that schema defines welcome-extra-info as type string.""" + schema_path = os.path.join(os.path.dirname(__file__), "..", "config", "schema.yaml") + with open(schema_path) as f: + schema = yaml.safe_load(f) - try: - monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + # Global level + global_props = schema["properties"] + assert "welcome-extra-info" in global_props + assert global_props["welcome-extra-info"]["type"] == "string" - config_obj = Config() - assert config_obj.root_data["welcome-extra-info"] == 12345 - finally: - shutil.rmtree(temp_dir) + # Per-repo level + repo_props = schema["properties"]["repositories"]["additionalProperties"]["properties"] + assert "welcome-extra-info" in repo_props + assert repo_props["welcome-extra-info"]["type"] == "string" def test_welcome_extra_info_repository_level( self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch @@ -1059,19 +1058,16 @@ def test_welcome_extra_info_repository_level( finally: shutil.rmtree(temp_dir) - def test_welcome_extra_info_max_length( - self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch - ) -> None: - """Test that welcome-extra-info exceeding 10240 chars loads (runtime guard handles it).""" - config = valid_minimal_config.copy() - config["welcome-extra-info"] = "x" * 10241 - - temp_dir = self.create_temp_config_dir_and_data(config) + def test_welcome_extra_info_schema_defines_max_length(self) -> None: + """Test that schema defines welcome-extra-info with maxLength 10240.""" + schema_path = os.path.join(os.path.dirname(__file__), "..", "config", "schema.yaml") + with open(schema_path) as f: + schema = yaml.safe_load(f) - try: - monkeypatch.setenv("WEBHOOK_SERVER_DATA_DIR", temp_dir) + # Global level + global_props = schema["properties"] + assert global_props["welcome-extra-info"]["maxLength"] == 10240 - config_obj = Config() - assert len(config_obj.root_data["welcome-extra-info"]) == 10241 - finally: - shutil.rmtree(temp_dir) + # Per-repo level + repo_props = schema["properties"]["repositories"]["additionalProperties"]["properties"] + assert repo_props["welcome-extra-info"]["maxLength"] == 10240 diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 7969e5fd1..c20f746f7 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2931,8 +2931,9 @@ def github_webhook(self, process_github_webhook: GithubWebhook) -> GithubWebhook return process_github_webhook def test_config_welcome_extra_info_oversized(self, github_webhook: GithubWebhook) -> None: - """Test that oversized config value is cleared by the guard.""" - oversized_value = "x" * 10241 + """Test that oversized config value is cleared by the guard using byte length.""" + # Use 2-byte UTF-8 characters (é) so 5121 chars = 10242 bytes > 10240 limit + oversized_value = "é" * 5121 github_webhook.config.get_value = MagicMock( side_effect=lambda value="", return_on_none=None, extra_dict=None: ( oversized_value if value == "welcome-extra-info" else MagicMock() From 72ed9e42795b42d20a38a315d930401e914af832 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 09:59:08 +0300 Subject: [PATCH 04/10] fix: compute UTF-8 byte length once in size guard Signed-off-by: rnetser Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index c040843e9..14655b3f7 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -1127,10 +1127,11 @@ def _sanitize_item(item: Any) -> str: f"welcome-extra-info must be a string, got {type(self.welcome_extra_info).__name__}. Ignoring." ) self.welcome_extra_info = "" - elif len(self.welcome_extra_info.encode("utf-8")) > 10240: + else: _byte_len = len(self.welcome_extra_info.encode("utf-8")) - self.logger.warning(f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring.") - self.welcome_extra_info = "" + if _byte_len > 10240: + self.logger.warning(f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring.") + self.welcome_extra_info = "" async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: """Add dynamic entries to trusted-committers list. From eb57db789d7f7dafc3951e9abd4aa7fbffa804dc Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 11:25:14 +0300 Subject: [PATCH 05/10] fix: add log prefix to welcome-extra-info guard warnings Signed-off-by: rnetser Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 14655b3f7..ccd2f1ffb 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -1122,15 +1122,16 @@ def _sanitize_item(item: Any) -> str: "welcome-extra-info", return_on_none="", extra_dict=repository_config ) + _prefix = getattr(self, "log_prefix", "") if not isinstance(self.welcome_extra_info, str): - self.logger.warning( - f"welcome-extra-info must be a string, got {type(self.welcome_extra_info).__name__}. Ignoring." - ) + _type = type(self.welcome_extra_info).__name__ + self.logger.warning(f"{_prefix} welcome-extra-info must be a string, got {_type}. Ignoring.") self.welcome_extra_info = "" else: _byte_len = len(self.welcome_extra_info.encode("utf-8")) if _byte_len > 10240: - self.logger.warning(f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring.") + _msg = f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring." + self.logger.warning(f"{_prefix} {_msg}") self.welcome_extra_info = "" async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: From 91ec45a3917bbf20156a29359dca7726b2776c96 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 12:55:17 +0300 Subject: [PATCH 06/10] fix: address human review comments on PR #1147 - Revert manual docs/ edits (regenerate with docsfy) - Add load_welcome_extra_info_from_file() call in /reprocess path - Rename _load_welcome_extra_info_from_file to public method - Extract 10240 and filename string to module constants - DRY mock_to_thread into autouse class fixture - Use constant in log message and fix Generator type hint Signed-off-by: rnetser Assisted-by: Claude Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- docs/container-and-pypi-workflows.md | 6 +- docs/testing-and-maintenance.md | 1 - webhook_server/libs/github_api.py | 39 +++++------ .../libs/handlers/pull_request_handler.py | 5 +- webhook_server/tests/test_github_api.py | 68 +++++++------------ 5 files changed, 51 insertions(+), 68 deletions(-) diff --git a/docs/container-and-pypi-workflows.md b/docs/container-and-pypi-workflows.md index 6fdfb1c16..30d386a35 100644 --- a/docs/container-and-pypi-workflows.md +++ b/docs/container-and-pypi-workflows.md @@ -82,7 +82,11 @@ The tag-selection logic in the code is: ```python if is_merged: pull_request_branch = pull_request.base.ref - tag = pull_request_branch if pull_request_branch not in (OTHER_MAIN_BRANCH, "main") else self.container_tag + tag = ( + pull_request_branch + if pull_request_branch not in (OTHER_MAIN_BRANCH, "main") + else self.container_tag + ) else: tag = f"pr-{pull_request.number}" ``` diff --git a/docs/testing-and-maintenance.md b/docs/testing-and-maintenance.md index 93aa90eaf..d7c33e3e6 100644 --- a/docs/testing-and-maintenance.md +++ b/docs/testing-and-maintenance.md @@ -75,7 +75,6 @@ A representative example from `webhook_server/tests/test_app.py` posts a webhook def client(self) -> TestClient: return TestClient(FASTAPI_APP) - @patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"}) @patch("webhook_server.app.GithubWebhook") def test_process_webhook_success( diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index ccd2f1ffb..0dbdd0641 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -63,6 +63,8 @@ ) _SHA_PATTERN = re.compile(r"^[0-9a-f]{40}$") +_WELCOME_EXTRA_INFO_MAX_BYTES: int = 10240 +_WELCOME_EXTRA_INFO_FILENAME: str = ".github-webhook-server-welcome-message.md" class CountingRequester: @@ -696,7 +698,7 @@ async def process(self) -> Any: "reopened", "ready_for_review", ): - await self._load_welcome_extra_info_from_file() + await self.load_welcome_extra_info_from_file() # Clone repository for local file processing (OWNERS, changed files) # For check_run, status, and pull_request_review_thread events, @@ -1129,8 +1131,9 @@ def _sanitize_item(item: Any) -> str: self.welcome_extra_info = "" else: _byte_len = len(self.welcome_extra_info.encode("utf-8")) - if _byte_len > 10240: - _msg = f"welcome-extra-info exceeds 10KB limit ({_byte_len} bytes). Ignoring." + if _byte_len > _WELCOME_EXTRA_INFO_MAX_BYTES: + _max = _WELCOME_EXTRA_INFO_MAX_BYTES + _msg = f"welcome-extra-info exceeds {_max}-byte limit ({_byte_len} bytes). Ignoring." self.logger.warning(f"{_prefix} {_msg}") self.welcome_extra_info = "" @@ -1167,25 +1170,25 @@ async def _build_trusted_committers(self, api_users: list[str | None] | None = N self.logger.debug(f"{self.log_prefix} Trusted committers: {self.security_trusted_committers}") - async def _load_welcome_extra_info_from_file(self) -> None: - """Load welcome extra info from .github-webhook-server-welcome-message.md in the repo. + async def load_welcome_extra_info_from_file(self) -> None: + """Load welcome extra info from the repo-level welcome message file. This file takes priority over all config-based welcome-extra-info settings. - File must be UTF-8 and at most 10KB. + File must be UTF-8 and at most _WELCOME_EXTRA_INFO_MAX_BYTES bytes (10 KB). """ try: _path = await github_api_call( - lambda: self.repository.get_contents(".github-webhook-server-welcome-message.md"), + lambda: self.repository.get_contents(_WELCOME_EXTRA_INFO_FILENAME), logger=self.logger, log_prefix=self.log_prefix, ) content_file = _path[0] if isinstance(_path, list) else _path file_size = await github_api_call(lambda: content_file.size, logger=self.logger, log_prefix=self.log_prefix) - if file_size > 10240: + if file_size > _WELCOME_EXTRA_INFO_MAX_BYTES: self.logger.warning( - f"{self.log_prefix} .github-webhook-server-welcome-message.md is too large " - f"({file_size} bytes, max 10240). Skipping file, using config value." + f"{self.log_prefix} {_WELCOME_EXTRA_INFO_FILENAME} is too large " + f"({file_size} bytes, max {_WELCOME_EXTRA_INFO_MAX_BYTES}). Skipping file, using config value." ) return @@ -1196,26 +1199,20 @@ async def _load_welcome_extra_info_from_file(self) -> None: self.welcome_extra_info = decoded if decoded: self.logger.info( - f"{self.log_prefix} Loaded welcome extra info from .github-webhook-server-welcome-message.md " + f"{self.log_prefix} Loaded welcome extra info from {_WELCOME_EXTRA_INFO_FILENAME} " f"({len(decoded)} chars)" ) else: self.logger.info( - f"{self.log_prefix} Empty .github-webhook-server-welcome-message.md found, " + f"{self.log_prefix} Empty {_WELCOME_EXTRA_INFO_FILENAME} found, " "suppressing config-based welcome extra info" ) except UnknownObjectException: - self.logger.debug( - f"{self.log_prefix} .github-webhook-server-welcome-message.md not found, using config value" - ) + self.logger.debug(f"{self.log_prefix} {_WELCOME_EXTRA_INFO_FILENAME} not found, using config value") except UnicodeDecodeError: - self.logger.warning( - f"{self.log_prefix} .github-webhook-server-welcome-message.md is not valid UTF-8, skipping file" - ) + self.logger.warning(f"{self.log_prefix} {_WELCOME_EXTRA_INFO_FILENAME} is not valid UTF-8, skipping file") except Exception: - self.logger.exception( - f"{self.log_prefix} Error loading .github-webhook-server-welcome-message.md, using config value" - ) + self.logger.exception(f"{self.log_prefix} Error loading {_WELCOME_EXTRA_INFO_FILENAME}, using config value") async def get_pull_request(self, number: int | None = None) -> PullRequest | None: if number: diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 19e037d98..b35b78fb8 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1934,7 +1934,7 @@ async def regenerate_welcome_message(self, pull_request: PullRequest) -> None: If a welcome message exists, it will be updated. If no welcome message exists, a new one will be created. """ - await self.github_webhook._load_welcome_extra_info_from_file() + await self.github_webhook.load_welcome_extra_info_from_file() welcome_msg = self._prepare_welcome_comment() def find_and_update_welcome_comment() -> bool: @@ -1972,6 +1972,9 @@ async def process_new_or_reprocess_pull_request(self, pull_request: PullRequest) """ tasks: list[Coroutine[Any, Any, Any]] = [] + # Load file-based welcome extra info before building the welcome message + await self.github_webhook.load_welcome_extra_info_from_file() + # Add welcome message if it doesn't exist yet if not await self._welcome_comment_exists(pull_request=pull_request): self.logger.info(f"{self.log_prefix} Adding welcome message to PR") diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index c20f746f7..f1757f40d 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1,7 +1,7 @@ import asyncio import os import tempfile -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Generator from pathlib import Path from typing import Any from unittest.mock import AsyncMock, MagicMock, Mock, patch @@ -2815,7 +2815,7 @@ def mock_get_value(value: str, return_on_none: Any = None, extra_dict: Any = Non class TestLoadWelcomeExtraInfoFromFile: - """Tests for _load_welcome_extra_info_from_file.""" + """Tests for load_welcome_extra_info_from_file.""" @pytest.fixture() def github_webhook(self, process_github_webhook: GithubWebhook) -> GithubWebhook: @@ -2825,18 +2825,22 @@ def github_webhook(self, process_github_webhook: GithubWebhook) -> GithubWebhook gw.repository = MagicMock() return gw + @pytest.fixture(autouse=True) + def patch_to_thread(self) -> Generator[None]: + async def mock_to_thread(func: Any, *args: Any, **kwargs: Any) -> Any: + return func(*args, **kwargs) if callable(func) else func + + with patch("asyncio.to_thread", side_effect=mock_to_thread): + yield + @pytest.mark.asyncio() async def test_load_from_file_success(self, github_webhook: GithubWebhook) -> None: content_file = MagicMock() content_file.size = 100 content_file.decoded_content = b"# Custom welcome\nSome info" - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.return_value = content_file - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.return_value = content_file + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "# Custom welcome\nSome info" @@ -2845,34 +2849,22 @@ async def test_load_from_file_too_large(self, github_webhook: GithubWebhook) -> content_file = MagicMock() content_file.size = 20000 - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.return_value = content_file - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.return_value = content_file + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "config value" @pytest.mark.asyncio() async def test_load_from_file_not_found(self, github_webhook: GithubWebhook) -> None: - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.side_effect = UnknownObjectException(404, "Not found", None) - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.side_effect = UnknownObjectException(404, "Not found", None) + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "config value" @pytest.mark.asyncio() async def test_load_from_file_exception(self, github_webhook: GithubWebhook) -> None: - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.side_effect = Exception("API error") - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.side_effect = Exception("API error") + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "config value" @@ -2882,12 +2874,8 @@ async def test_load_from_file_empty(self, github_webhook: GithubWebhook) -> None content_file.size = 0 content_file.decoded_content = b"" - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.return_value = content_file - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.return_value = content_file + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "" @@ -2897,12 +2885,8 @@ async def test_load_from_file_list_response(self, github_webhook: GithubWebhook) content_file.size = 50 content_file.decoded_content = b"List content" - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.return_value = [content_file] - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.return_value = [content_file] + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "List content" @@ -2912,12 +2896,8 @@ async def test_load_from_file_unicode_error(self, github_webhook: GithubWebhook) content_file.size = 10 content_file.decoded_content = b"\xff\xfe invalid" - async def mock_to_thread(func, *args, **kwargs): - return func(*args, **kwargs) if callable(func) else func - - with patch("asyncio.to_thread", side_effect=mock_to_thread): - github_webhook.repository.get_contents.return_value = content_file - await github_webhook._load_welcome_extra_info_from_file() + github_webhook.repository.get_contents.return_value = content_file + await github_webhook.load_welcome_extra_info_from_file() assert github_webhook.welcome_extra_info == "config value" From 7198a7b05943b8ac706acf7985648ce30afc24f6 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 14:02:53 +0300 Subject: [PATCH 07/10] =?UTF-8?q?fix:=20address=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20stable=20interfaces=20+=20DRY=20test=20fixtures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add .github-webhook-server-welcome-message.md to stable interfaces list in AGENTS.md - Extract _create_mock_github_webhook() and _create_mock_owners_file_handler() as module-level helpers in test_pull_request_handler.py to eliminate ~55 lines of duplication - Add docstrings to all test methods in TestPrepareExtraInfoWelcomeSection and TestLoadWelcomeExtraInfoFromFile Signed-off-by: Ruth Netser Assisted-by: Claude Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- AGENTS.md | 2 +- webhook_server/tests/test_github_api.py | 7 + .../tests/test_pull_request_handler.py | 182 +++++++----------- 3 files changed, 79 insertions(+), 112 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d826c1201..3e82543f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,7 @@ Handlers in `webhook_server/libs/handlers/` process events; config via YAML with See `docs/` for generated architecture docs (regenerate with docsfy — **NEVER edit `docs/` manually**). - Stack: Python 3.13, FastAPI, PyGithub, gql, aiohttp -- Internal APIs — no backward compat; only `config.yaml`, `.github-webhook-server.yaml`, and webhook payloads are stable +- Internal APIs — no backward compat; only `config.yaml`, `.github-webhook-server.yaml`, `.github-webhook-server-welcome-message.md`, and webhook payloads are stable - Config: `webhook_server/libs/config.py` (schema: `webhook_server/config/schema.yaml`) - GitHub API: `webhook_server/libs/github_api.py` — PyGithub REST v3, multi-token failover - Log viewer: `webhook_server/web/log_viewer.py` — WebSocket streaming diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index f1757f40d..35ced39a7 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2835,6 +2835,7 @@ async def mock_to_thread(func: Any, *args: Any, **kwargs: Any) -> Any: @pytest.mark.asyncio() async def test_load_from_file_success(self, github_webhook: GithubWebhook) -> None: + """Test successful loading of welcome extra info from repository file.""" content_file = MagicMock() content_file.size = 100 content_file.decoded_content = b"# Custom welcome\nSome info" @@ -2846,6 +2847,7 @@ async def test_load_from_file_success(self, github_webhook: GithubWebhook) -> No @pytest.mark.asyncio() async def test_load_from_file_too_large(self, github_webhook: GithubWebhook) -> None: + """Test that oversized files are rejected with a warning.""" content_file = MagicMock() content_file.size = 20000 @@ -2856,6 +2858,7 @@ async def test_load_from_file_too_large(self, github_webhook: GithubWebhook) -> @pytest.mark.asyncio() async def test_load_from_file_not_found(self, github_webhook: GithubWebhook) -> None: + """Test graceful handling when welcome message file does not exist.""" github_webhook.repository.get_contents.side_effect = UnknownObjectException(404, "Not found", None) await github_webhook.load_welcome_extra_info_from_file() @@ -2863,6 +2866,7 @@ async def test_load_from_file_not_found(self, github_webhook: GithubWebhook) -> @pytest.mark.asyncio() async def test_load_from_file_exception(self, github_webhook: GithubWebhook) -> None: + """Test graceful handling of unexpected exceptions during file loading.""" github_webhook.repository.get_contents.side_effect = Exception("API error") await github_webhook.load_welcome_extra_info_from_file() @@ -2870,6 +2874,7 @@ async def test_load_from_file_exception(self, github_webhook: GithubWebhook) -> @pytest.mark.asyncio() async def test_load_from_file_empty(self, github_webhook: GithubWebhook) -> None: + """Test that empty file content is handled correctly.""" content_file = MagicMock() content_file.size = 0 content_file.decoded_content = b"" @@ -2881,6 +2886,7 @@ async def test_load_from_file_empty(self, github_webhook: GithubWebhook) -> None @pytest.mark.asyncio() async def test_load_from_file_list_response(self, github_webhook: GithubWebhook) -> None: + """Test handling of unexpected list response from GitHub API.""" content_file = MagicMock() content_file.size = 50 content_file.decoded_content = b"List content" @@ -2892,6 +2898,7 @@ async def test_load_from_file_list_response(self, github_webhook: GithubWebhook) @pytest.mark.asyncio() async def test_load_from_file_unicode_error(self, github_webhook: GithubWebhook) -> None: + """Test handling of unicode decode errors in file content.""" content_file = MagicMock() content_file.size = 10 content_file.decoded_content = b"\xff\xfe invalid" diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 17fab1bd1..38490de95 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -47,72 +47,81 @@ def _owners_data_coroutine(return_value: dict | None = None) -> _AwaitableValue: return _AwaitableValue(return_value) +def _create_mock_github_webhook() -> Mock: + """Create a mock GithubWebhook instance for testing.""" + mock_webhook = Mock(spec=GithubWebhook) + mock_webhook.hook_data = { + "action": "opened", + "pull_request": {"number": 123, "merged": False, "title": "Test PR"}, + "sender": {"login": "test-user"}, + "label": {"name": "bug"}, + } + mock_webhook.logger = MagicMock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository_full_name = "test-org/test-repo" + mock_webhook.repository = Mock() + mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" + mock_webhook.parent_committer = "test-user" + mock_webhook.auto_verified_and_merged_users = ["test-user"] + mock_webhook.create_issue_for_new_pr = True + mock_webhook.verified_job = True + mock_webhook.build_and_push_container = True + mock_webhook.container_repository_and_tag = Mock(return_value="test-repo:pr-123") + mock_webhook.can_be_merged_required_labels = [] + mock_webhook.set_auto_merge_prs = [] + mock_webhook.auto_merge_enabled = True + mock_webhook.container_repository = "docker.io/org/repo" + mock_webhook.conventional_title = False + mock_webhook.minimum_lgtm = 1 + mock_webhook.container_repository_username = "test-user" + mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret + mock_webhook.github_api = Mock() + mock_webhook.tox = True + mock_webhook.pre_commit = True + mock_webhook.python_module_install = False + mock_webhook.pypi = False + mock_webhook.token = TEST_GITHUB_TOKEN + mock_webhook.auto_verify_cherry_picked_prs = True + mock_webhook.cherry_pick_assign_to_pr_author = True + mock_webhook.last_commit = Mock() + mock_webhook.ctx = None + mock_webhook.enabled_labels = None + mock_webhook.custom_check_runs = [] + mock_webhook.ai_features = None + mock_webhook.required_conversation_resolution = False + mock_webhook.security_suspicious_paths = [] + mock_webhook.security_committer_identity_check = True + mock_webhook.security_mandatory = True + mock_webhook.last_committer = "test-user" + mock_webhook.welcome_extra_info = "" + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + return mock_webhook + + +def _create_mock_owners_file_handler() -> Mock: + """Create a mock OwnersFileHandler instance for testing.""" + mock_handler = Mock(spec=OwnersFileHandler) + mock_handler.all_pull_request_approvers = ["approver1", "approver2"] + mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] + mock_handler.root_approvers = ["root-approver"] + mock_handler.root_reviewers = ["root-reviewer"] + mock_handler.assign_reviewers = AsyncMock() + return mock_handler + + class TestPullRequestHandler: """Test suite for PullRequestHandler class.""" @pytest.fixture def mock_github_webhook(self) -> Mock: """Create a mock GithubWebhook instance.""" - mock_webhook = Mock(spec=GithubWebhook) - mock_webhook.hook_data = { - "action": "opened", - "pull_request": {"number": 123, "merged": False, "title": "Test PR"}, - "sender": {"login": "test-user"}, - "label": {"name": "bug"}, - } - mock_webhook.logger = MagicMock() - mock_webhook.log_prefix = "[TEST]" - mock_webhook.repository_full_name = "test-org/test-repo" - mock_webhook.repository = Mock() - mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" - mock_webhook.parent_committer = "test-user" - mock_webhook.auto_verified_and_merged_users = ["test-user"] - mock_webhook.create_issue_for_new_pr = True - mock_webhook.verified_job = True - mock_webhook.build_and_push_container = True - mock_webhook.container_repository_and_tag = Mock(return_value="test-repo:pr-123") - mock_webhook.can_be_merged_required_labels = [] - mock_webhook.set_auto_merge_prs = [] - mock_webhook.auto_merge_enabled = True - mock_webhook.container_repository = "docker.io/org/repo" - # New attributes for coverage - mock_webhook.conventional_title = False - mock_webhook.minimum_lgtm = 1 - mock_webhook.container_repository_username = "test-user" - mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret - mock_webhook.github_api = Mock() - mock_webhook.tox = True - mock_webhook.pre_commit = True - mock_webhook.python_module_install = False - mock_webhook.pypi = False - mock_webhook.token = TEST_GITHUB_TOKEN - mock_webhook.auto_verify_cherry_picked_prs = True - mock_webhook.cherry_pick_assign_to_pr_author = True - mock_webhook.last_commit = Mock() - mock_webhook.ctx = None - mock_webhook.enabled_labels = None # Default: all labels enabled - mock_webhook.custom_check_runs = [] - mock_webhook.ai_features = None - mock_webhook.required_conversation_resolution = False - mock_webhook.security_suspicious_paths = [] - mock_webhook.security_committer_identity_check = True - mock_webhook.security_mandatory = True - mock_webhook.last_committer = "test-user" - mock_webhook.welcome_extra_info = "" - mock_webhook.config = Mock() - mock_webhook.config.get_value = Mock(return_value=None) - return mock_webhook + return _create_mock_github_webhook() @pytest.fixture def mock_owners_file_handler(self) -> Mock: """Create a mock OwnersFileHandler instance.""" - mock_handler = Mock(spec=OwnersFileHandler) - mock_handler.all_pull_request_approvers = ["approver1", "approver2"] - mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] - mock_handler.root_approvers = ["root-approver"] - mock_handler.root_reviewers = ["root-reviewer"] - mock_handler.assign_reviewers = AsyncMock() - return mock_handler + return _create_mock_owners_file_handler() @pytest.fixture def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> PullRequestHandler: @@ -3304,65 +3313,12 @@ class TestPrepareExtraInfoWelcomeSection: @pytest.fixture def mock_github_webhook(self) -> Mock: """Create a mock GithubWebhook instance.""" - mock_webhook = Mock(spec=GithubWebhook) - mock_webhook.hook_data = { - "action": "opened", - "pull_request": {"number": 123, "merged": False, "title": "Test PR"}, - "sender": {"login": "test-user"}, - "label": {"name": "bug"}, - } - mock_webhook.logger = MagicMock() - mock_webhook.log_prefix = "[TEST]" - mock_webhook.repository_full_name = "test-org/test-repo" - mock_webhook.repository = Mock() - mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" - mock_webhook.parent_committer = "test-user" - mock_webhook.auto_verified_and_merged_users = ["test-user"] - mock_webhook.create_issue_for_new_pr = True - mock_webhook.verified_job = True - mock_webhook.build_and_push_container = True - mock_webhook.container_repository_and_tag = Mock(return_value="test-repo:pr-123") - mock_webhook.can_be_merged_required_labels = [] - mock_webhook.set_auto_merge_prs = [] - mock_webhook.auto_merge_enabled = True - mock_webhook.container_repository = "docker.io/org/repo" - mock_webhook.conventional_title = False - mock_webhook.minimum_lgtm = 1 - mock_webhook.container_repository_username = "test-user" - mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret - mock_webhook.github_api = Mock() - mock_webhook.tox = True - mock_webhook.pre_commit = True - mock_webhook.python_module_install = False - mock_webhook.pypi = False - mock_webhook.token = TEST_GITHUB_TOKEN - mock_webhook.auto_verify_cherry_picked_prs = True - mock_webhook.cherry_pick_assign_to_pr_author = True - mock_webhook.last_commit = Mock() - mock_webhook.ctx = None - mock_webhook.enabled_labels = None - mock_webhook.custom_check_runs = [] - mock_webhook.ai_features = None - mock_webhook.required_conversation_resolution = False - mock_webhook.security_suspicious_paths = [] - mock_webhook.security_committer_identity_check = True - mock_webhook.security_mandatory = True - mock_webhook.last_committer = "test-user" - mock_webhook.welcome_extra_info = "" - mock_webhook.config = Mock() - mock_webhook.config.get_value = Mock(return_value=None) - return mock_webhook + return _create_mock_github_webhook() @pytest.fixture def mock_owners_file_handler(self) -> Mock: """Create a mock OwnersFileHandler instance.""" - mock_handler = Mock(spec=OwnersFileHandler) - mock_handler.all_pull_request_approvers = ["approver1", "approver2"] - mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] - mock_handler.root_approvers = ["root-approver"] - mock_handler.root_reviewers = ["root-reviewer"] - mock_handler.assign_reviewers = AsyncMock() - return mock_handler + return _create_mock_owners_file_handler() @pytest.fixture def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> PullRequestHandler: @@ -3373,22 +3329,26 @@ def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handl return handler def test_extra_info_present(self, pull_request_handler: PullRequestHandler) -> None: + """Test extra info section is rendered when welcome_extra_info is set.""" pull_request_handler.github_webhook.welcome_extra_info = "Custom info text" result = pull_request_handler._prepare_extra_info_welcome_section assert "### \U0001f4cc Additional Information\n\n" in result assert "Custom info text" in result def test_extra_info_empty(self, pull_request_handler: PullRequestHandler) -> None: + """Test extra info section is empty when welcome_extra_info is empty.""" pull_request_handler.github_webhook.welcome_extra_info = "" result = pull_request_handler._prepare_extra_info_welcome_section assert result == "" def test_extra_info_none(self, pull_request_handler: PullRequestHandler) -> None: + """Test extra info section is empty when welcome_extra_info is None.""" pull_request_handler.github_webhook.welcome_extra_info = None result = pull_request_handler._prepare_extra_info_welcome_section assert result == "" def test_extra_info_multiline_markdown(self, pull_request_handler: PullRequestHandler) -> None: + """Test extra info section renders multiline markdown correctly.""" pull_request_handler.github_webhook.welcome_extra_info = "**Bold** and *italic*\n- Item 1\n- Item 2" result = pull_request_handler._prepare_extra_info_welcome_section assert "### \U0001f4cc Additional Information\n\n" in result From 5513e10fb836c2355aae9efd00191beaa9f28085 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 14:22:26 +0300 Subject: [PATCH 08/10] fix: defer file loading until welcome comment is needed Move load_welcome_extra_info_from_file() inside the welcome comment existence check to avoid unnecessary GitHub API call during /reprocess when welcome comment already exists. Signed-off-by: Ruth Netser Assisted-by: Claude Co-authored-by: PI (claude-opus-4-6) Signed-off-by: rnetser --- webhook_server/libs/handlers/pull_request_handler.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index b35b78fb8..df9801471 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1972,11 +1972,10 @@ async def process_new_or_reprocess_pull_request(self, pull_request: PullRequest) """ tasks: list[Coroutine[Any, Any, Any]] = [] - # Load file-based welcome extra info before building the welcome message - await self.github_webhook.load_welcome_extra_info_from_file() - # Add welcome message if it doesn't exist yet if not await self._welcome_comment_exists(pull_request=pull_request): + # Load file-based welcome extra info only when building the welcome message + await self.github_webhook.load_welcome_extra_info_from_file() self.logger.info(f"{self.log_prefix} Adding welcome message to PR") welcome_msg = self._prepare_welcome_comment() tasks.append( From 2c99e78a08f7dc32185539e2a7089cd814bd42eb Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 15:26:55 +0300 Subject: [PATCH 09/10] fix: extract welcome-extra-info to $defs/$ref in schema Move welcome-extra-info definition to $defs section and use $ref at both global and per-repo levels, matching the pattern used by ai-features and security-checks. Single source of truth for type/maxLength. Signed-off-by: Ruth Netser Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/config/schema.yaml | 21 ++++++++--------- webhook_server/tests/test_config_schema.py | 26 +++++++++++----------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index ffad8f448..acaa37830 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -106,6 +106,13 @@ $defs: items: type: string additionalProperties: false + welcome-extra-info: + type: string + maxLength: 10240 + description: | + Additional information to display at the end of the PR welcome message. + Content is injected as-is (markdown). + An empty string explicitly clears any inherited value. type: object properties: log-level: @@ -252,12 +259,7 @@ properties: ai-features: $ref: '#/$defs/ai-features' welcome-extra-info: - type: string - maxLength: 10240 - description: | - Additional information to display at the end of the PR welcome message (global default). - Content is injected as-is (markdown). Can be overridden per-repo. - An empty string explicitly clears any inherited value. + $ref: '#/$defs/welcome-extra-info' security-checks: $ref: '#/$defs/security-checks' labels: @@ -531,12 +533,7 @@ properties: type: string description: Required labels for PR to be marked as can-be-merged welcome-extra-info: - type: string - maxLength: 10240 - description: | - Additional information to display at the end of the PR welcome message (per-repo override). - Content is injected as-is (markdown). Overrides the global setting. - An empty string explicitly clears any inherited value. + $ref: '#/$defs/welcome-extra-info' conventional-title: type: string description: | diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index cb28eaf13..58ec99193 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -1025,20 +1025,25 @@ def test_welcome_extra_info_global_valid( shutil.rmtree(temp_dir) def test_welcome_extra_info_schema_defines_string_type(self) -> None: - """Test that schema defines welcome-extra-info as type string.""" + """Test that schema defines welcome-extra-info as type string via $ref.""" schema_path = os.path.join(os.path.dirname(__file__), "..", "config", "schema.yaml") with open(schema_path) as f: schema = yaml.safe_load(f) - # Global level + # Verify $defs definition + defs = schema["$defs"] + assert "welcome-extra-info" in defs + assert defs["welcome-extra-info"]["type"] == "string" + + # Global level uses $ref global_props = schema["properties"] assert "welcome-extra-info" in global_props - assert global_props["welcome-extra-info"]["type"] == "string" + assert global_props["welcome-extra-info"]["$ref"] == "#/$defs/welcome-extra-info" - # Per-repo level + # Per-repo level uses $ref repo_props = schema["properties"]["repositories"]["additionalProperties"]["properties"] assert "welcome-extra-info" in repo_props - assert repo_props["welcome-extra-info"]["type"] == "string" + assert repo_props["welcome-extra-info"]["$ref"] == "#/$defs/welcome-extra-info" def test_welcome_extra_info_repository_level( self, valid_minimal_config: dict[str, Any], monkeypatch: pytest.MonkeyPatch @@ -1059,15 +1064,10 @@ def test_welcome_extra_info_repository_level( shutil.rmtree(temp_dir) def test_welcome_extra_info_schema_defines_max_length(self) -> None: - """Test that schema defines welcome-extra-info with maxLength 10240.""" + """Test that schema defines welcome-extra-info with maxLength 10240 in $defs.""" schema_path = os.path.join(os.path.dirname(__file__), "..", "config", "schema.yaml") with open(schema_path) as f: schema = yaml.safe_load(f) - # Global level - global_props = schema["properties"] - assert global_props["welcome-extra-info"]["maxLength"] == 10240 - - # Per-repo level - repo_props = schema["properties"]["repositories"]["additionalProperties"]["properties"] - assert repo_props["welcome-extra-info"]["maxLength"] == 10240 + # maxLength is in the $defs definition (both levels reference it via $ref) + assert schema["$defs"]["welcome-extra-info"]["maxLength"] == 10240 From 634d8f85bd934de57a4eadc085a1305d02014408 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 2 Jul 2026 23:20:15 +0300 Subject: [PATCH 10/10] fix: skip file loading for reopened pull requests Remove 'reopened' from the action list that triggers load_welcome_extra_info_from_file() in process(). The reopened handler does not build a welcome comment, so the file fetch was an unnecessary API call. Signed-off-by: Ruth Netser Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 1 - 1 file changed, 1 deletion(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 0dbdd0641..38c72c12c 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -695,7 +695,6 @@ async def process(self) -> Any: if self.github_event == "pull_request" and self.hook_data.get("action") in ( "opened", - "reopened", "ready_for_review", ): await self.load_welcome_extra_info_from_file()