From 5eea17360e4fde4f4150fcae95ba5552ded68cf0 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 16 Jun 2026 14:16:56 +0300 Subject: [PATCH 1/3] fix: add /retest support for security checks Security checks (security-committer-identity, security-suspicious-paths) were missing from the supported retest list and dispatch map, causing /retest to return "No ... configured for this repository". - Add security checks to _current_pull_request_supported_retest - Add security runners to run_retests dispatch map Closes #1122 --- webhook_server/libs/github_api.py | 9 +++++++++ webhook_server/libs/handlers/runner_handler.py | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 457e23d4e..64f87853e 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -39,6 +39,8 @@ OTHER_MAIN_BRANCH, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, + SECURITY_COMMITTER_IDENTITY_STR, + SECURITY_SUSPICIOUS_PATHS_STR, SUCCESS_STR, TOX_STR, ) @@ -1413,6 +1415,13 @@ def _current_pull_request_supported_retest(self) -> list[str]: check_name = custom_check["name"] current_pull_request_supported_retest.append(check_name) + # Add security checks when enabled + if self.security_committer_identity_check: + current_pull_request_supported_retest.append(SECURITY_COMMITTER_IDENTITY_STR) + + if self.security_suspicious_paths: + current_pull_request_supported_retest.append(SECURITY_SUSPICIOUS_PATHS_STR) + return current_pull_request_supported_retest async def cleanup(self) -> None: diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index c8d49e83f..0b1db15b4 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1971,6 +1971,13 @@ async def run_retests(self, supported_retests: list[str], pull_request: PullRequ check_key = custom_check["name"] _retests_to_func_map[check_key] = partial(self.run_custom_check, check_config=custom_check) + # Add security checks to the retest map + # Security methods don't take pull_request param, so wrap with lambda + _retests_to_func_map[SECURITY_COMMITTER_IDENTITY_STR] = lambda _pull_request: ( + self.run_security_committer_identity() + ) + _retests_to_func_map[SECURITY_SUSPICIOUS_PATHS_STR] = lambda _pull_request: self.run_security_suspicious_paths() + tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] scheduled_tests: list[str] = [] for _test in supported_retests: From 64f72302dbd51c618af701ce114c2235da9c8bcd Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 16 Jun 2026 14:27:19 +0300 Subject: [PATCH 2/3] fix: lambda kwarg mismatch in security retest dispatch The lambda wrappers used `_pull_request` but the caller passes `pull_request=` as a keyword arg. Python matches kwargs by name, so this would TypeError at runtime. Switch to **_kwargs. Also add tests for security retest support list inclusion/exclusion. --- .../libs/handlers/runner_handler.py | 6 +- webhook_server/tests/test_github_api.py | 71 +++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 0b1db15b4..e9b415efd 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1973,10 +1973,8 @@ async def run_retests(self, supported_retests: list[str], pull_request: PullRequ # Add security checks to the retest map # Security methods don't take pull_request param, so wrap with lambda - _retests_to_func_map[SECURITY_COMMITTER_IDENTITY_STR] = lambda _pull_request: ( - self.run_security_committer_identity() - ) - _retests_to_func_map[SECURITY_SUSPICIOUS_PATHS_STR] = lambda _pull_request: self.run_security_suspicious_paths() + _retests_to_func_map[SECURITY_COMMITTER_IDENTITY_STR] = lambda **_kwargs: self.run_security_committer_identity() + _retests_to_func_map[SECURITY_SUSPICIOUS_PATHS_STR] = lambda **_kwargs: self.run_security_suspicious_paths() tasks: list[Coroutine[Any, Any, Any] | Task[Any]] = [] scheduled_tests: list[str] = [] diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index c0a1bf4a4..35b4ead4c 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -15,6 +15,7 @@ from webhook_server.libs.github_api import GithubWebhook from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.tests.conftest import TEST_GITHUB_TOKEN +from webhook_server.utils.constants import SECURITY_COMMITTER_IDENTITY_STR, SECURITY_SUSPICIOUS_PATHS_STR class TestGithubWebhook: @@ -1319,6 +1320,76 @@ def test_current_pull_request_supported_retest_property( assert "pre-commit" in result assert "conventional-title" in result + def test_supported_retest_includes_security_checks( + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + ) -> None: + """Test that security checks are included in supported retests when enabled.""" + 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 = {} + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_get_repo_api.return_value = Mock() + + 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) + + gh.security_committer_identity_check = True + gh.security_suspicious_paths = ["Dockerfile", ".github/"] + gh.tox = False + gh.build_and_push_container = False + gh.pypi = False + gh.pre_commit = False + gh.conventional_title = False + gh.custom_check_runs = [] + + result = gh._current_pull_request_supported_retest + assert SECURITY_COMMITTER_IDENTITY_STR in result + assert SECURITY_SUSPICIOUS_PATHS_STR in result + + def test_supported_retest_excludes_disabled_security_checks( + self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + ) -> None: + """Test that disabled security checks are excluded from supported retests.""" + 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 = {} + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_get_repo_api.return_value = Mock() + + 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) + + gh.security_committer_identity_check = False + gh.security_suspicious_paths = [] + gh.tox = False + gh.build_and_push_container = False + gh.pypi = False + gh.pre_commit = False + gh.conventional_title = False + gh.custom_check_runs = [] + + result = gh._current_pull_request_supported_retest + assert SECURITY_COMMITTER_IDENTITY_STR not in result + assert SECURITY_SUSPICIOUS_PATHS_STR not in result + @pytest.mark.asyncio async def test_get_last_commit(self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock) -> None: """Test _get_last_commit method.""" From b0d7305875c2f5bb102b7035b8492c8eadcebf81 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 16 Jun 2026 14:36:18 +0300 Subject: [PATCH 3/3] fix: use parameterized dict type in security retest tests Change minimal_hook_data: dict to dict[str, Any] to comply with strict mypy disallow_any_generics setting. --- webhook_server/tests/test_github_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 35b4ead4c..e1357983a 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1321,7 +1321,7 @@ def test_current_pull_request_supported_retest_property( assert "conventional-title" in result def test_supported_retest_includes_security_checks( - self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + self, minimal_hook_data: dict[str, Any], minimal_headers: Headers, logger: Mock ) -> None: """Test that security checks are included in supported retests when enabled.""" with patch("webhook_server.libs.github_api.Config") as mock_config: @@ -1356,7 +1356,7 @@ def test_supported_retest_includes_security_checks( assert SECURITY_SUSPICIOUS_PATHS_STR in result def test_supported_retest_excludes_disabled_security_checks( - self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + self, minimal_hook_data: dict[str, Any], minimal_headers: Headers, logger: Mock ) -> None: """Test that disabled security checks are excluded from supported retests.""" with patch("webhook_server.libs.github_api.Config") as mock_config: