diff --git a/services/webhook/check_suite_handler.py b/services/webhook/check_suite_handler.py index 19670405..f0b79b62 100644 --- a/services/webhook/check_suite_handler.py +++ b/services/webhook/check_suite_handler.py @@ -582,7 +582,8 @@ async def handle_check_suite( for file_change in changed_files: allowed_to_edit_files.add(file_change["filename"]) impl_file_path = get_impl_file_from_pr_title(pr_title) - allowed_to_edit_files.add(impl_file_path) + if impl_file_path: + allowed_to_edit_files.add(impl_file_path) p += 5 add_log_message("Checked out the error log from the workflow run.", log_messages) diff --git a/services/webhook/new_pr_handler.py b/services/webhook/new_pr_handler.py index d6bec313..fe10d36e 100644 --- a/services/webhook/new_pr_handler.py +++ b/services/webhook/new_pr_handler.py @@ -305,6 +305,9 @@ async def handle_new_pr( # Extract target implementation file impl_file_path = get_impl_file_from_pr_title(pr_title) + if not impl_file_path: + raise ValueError(f"No file path found in PR title: {pr_title}") + if pr_title.startswith(SCHEDULE_PREFIX_INCREASE): base_args["impl_file_to_collect_coverage_from"] = impl_file_path test_dir_prefixes = repo_settings["test_dir_prefixes"] if repo_settings else [] diff --git a/services/webhook/test_successful_check_suite_handler.py b/services/webhook/test_successful_check_suite_handler.py index e0e7ff27..5dbdcdaa 100644 --- a/services/webhook/test_successful_check_suite_handler.py +++ b/services/webhook/test_successful_check_suite_handler.py @@ -40,7 +40,9 @@ def test_handle_successful_check_suite_with_pr( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) with patch( "services.webhook.successful_check_suite_handler.supabase" @@ -141,7 +143,9 @@ def test_handle_successful_check_suite_no_usage_record_found( mock_get_check_suites.return_value = [ {"app": {"name": "GitHub Actions"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["GitHub Actions"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["GitHub Actions"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "clean"} mock_check_skip_ci.return_value = False mock_get_files.return_value = [] @@ -239,7 +243,9 @@ def test_auto_merge_success( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "clean"} mock_check_skip_ci.return_value = False mock_get_files.return_value = [ @@ -368,7 +374,9 @@ def test_auto_merge_multiple_test_files_changed( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "clean"} mock_check_skip_ci.return_value = False mock_get_files.return_value = [ @@ -520,7 +528,9 @@ def test_auto_merge_with_non_test_files_allowed( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "clean"} mock_check_skip_ci.return_value = False mock_get_files.return_value = [ @@ -657,7 +667,9 @@ def test_auto_merge_blocked_skips_notification_when_checks_in_progress( }, {"app": {"name": "Cypress"}, "status": "in_progress", "conclusion": None}, ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "blocked"} with patch( @@ -726,7 +738,9 @@ def test_auto_merge_with_blocked_state( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "blocked"} with patch( @@ -796,7 +810,9 @@ def test_auto_merge_with_unstable_state( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "unstable"} mock_check_skip_ci.return_value = False mock_get_files.return_value = [ @@ -875,7 +891,9 @@ def test_auto_merge_with_unknown_state_no_comment( mock_get_check_suites.return_value = [ {"app": {"name": "CircleCI Checks"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["CircleCI Checks"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["CircleCI Checks"], strict=True + ) mock_get_pr.return_value = {"mergeable_state": "unknown"} with patch( @@ -935,7 +953,9 @@ def test_skip_ci_returns_early( mock_get_check_suites.return_value = [ {"app": {"name": "GitHub Actions"}, "status": "completed"} ] - mock_get_required_checks.return_value = StatusChecksResult(status_code=200, checks=["GitHub Actions"], strict=True) + mock_get_required_checks.return_value = StatusChecksResult( + status_code=200, checks=["GitHub Actions"], strict=True + ) mock_check_skip_ci.return_value = True with patch( diff --git a/utils/files/get_impl_file_from_pr_title.py b/utils/files/get_impl_file_from_pr_title.py index b329cf25..7e26cd4d 100644 --- a/utils/files/get_impl_file_from_pr_title.py +++ b/utils/files/get_impl_file_from_pr_title.py @@ -1,13 +1,16 @@ from utils.error.handle_exceptions import handle_exceptions +from utils.logging.logging_config import logger -@handle_exceptions(raise_on_error=True) +@handle_exceptions(default_return_value=None, raise_on_error=False) def get_impl_file_from_pr_title(pr_title: str): if not pr_title: - raise ValueError("pr_title is empty") + logger.warning("get_impl_file_from_pr_title: pr_title is empty") + return None last_token = pr_title.split()[-1] if "/" in last_token or "." in last_token: return last_token - raise ValueError(f"No file path found in PR title: {pr_title}") + logger.warning("No file path found in PR title: %s", pr_title) + return None diff --git a/utils/files/is_target_test_file.py b/utils/files/is_target_test_file.py index e5c4d344..bf828956 100644 --- a/utils/files/is_target_test_file.py +++ b/utils/files/is_target_test_file.py @@ -4,20 +4,36 @@ from utils.error.handle_exceptions import handle_exceptions from utils.files.get_impl_file_from_pr_title import get_impl_file_from_pr_title from utils.files.is_test_file import is_test_file +from utils.logging.logging_config import logger @handle_exceptions(default_return_value=True, raise_on_error=False) def is_target_test_file(file_path: str, base_args: BaseArgs): if not is_test_file(file_path): - # Return False (block edit) when file is not a test file + logger.info( + "is_target_test_file: %s is not a test file, blocking edit", file_path + ) return False pr_title = base_args.get("pr_title", "") if not pr_title: - # Return True (allow edit) when title is missing to avoid blocking GitAuto + logger.info("is_target_test_file: no pr_title, allowing edit for %s", file_path) return True implementation_file = get_impl_file_from_pr_title(pr_title) - target_filename = Path(implementation_file).stem + if not implementation_file: + logger.info( + "is_target_test_file: no file path in title, allowing edit for %s", + file_path, + ) + return True - return target_filename.lower() in file_path.lower() + target_filename = Path(implementation_file).stem + is_target = target_filename.lower() in file_path.lower() + logger.info( + "is_target_test_file: %s, target=%s, file_path=%s", + is_target, + target_filename, + file_path, + ) + return is_target diff --git a/utils/files/test_get_impl_file_from_pr_title.py b/utils/files/test_get_impl_file_from_pr_title.py index c3495879..094e81e0 100644 --- a/utils/files/test_get_impl_file_from_pr_title.py +++ b/utils/files/test_get_impl_file_from_pr_title.py @@ -1,5 +1,3 @@ -import pytest - from utils.files.get_impl_file_from_pr_title import get_impl_file_from_pr_title @@ -15,13 +13,11 @@ def test_achieve_coverage_prefix(): def test_non_matching_title(): title = "Fix bug in authentication" - with pytest.raises(ValueError): - get_impl_file_from_pr_title(title) + assert get_impl_file_from_pr_title(title) is None def test_empty_title(): - with pytest.raises(ValueError): - get_impl_file_from_pr_title("") + assert get_impl_file_from_pr_title("") is None def test_file_at_root(): @@ -31,5 +27,4 @@ def test_file_at_root(): def test_no_file_extension(): title = "Fix the Makefile" - with pytest.raises(ValueError): - get_impl_file_from_pr_title(title) + assert get_impl_file_from_pr_title(title) is None