From 8e3b0ce51254620eda08176673cc62a3d01acb41 Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Thu, 26 Feb 2026 12:48:19 -0800 Subject: [PATCH] Fix review handler rejecting dashboard-triggered PR reviews by checking branch prefix instead of pr_creator --- .../github/files/test_get_eslint_config.py | 76 +++++-------------- services/webhook/review_run_handler.py | 9 ++- services/webhook/test_new_pr_handler.py | 68 +++++++++++++---- services/webhook/test_review_run_handler.py | 3 +- 4 files changed, 82 insertions(+), 74 deletions(-) diff --git a/services/github/files/test_get_eslint_config.py b/services/github/files/test_get_eslint_config.py index 5594b439..f4e3d9e8 100644 --- a/services/github/files/test_get_eslint_config.py +++ b/services/github/files/test_get_eslint_config.py @@ -66,9 +66,7 @@ def base_args(): def test_get_eslint_config_finds_eslintrc_json(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc.json": @@ -92,9 +90,7 @@ def test_get_eslint_config_finds_eslintrc_js(base_args): } };""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc.js": @@ -115,9 +111,7 @@ def test_get_eslint_config_finds_eslintrc_yml(base_args): rules: no-console: warn""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc.yml": @@ -138,9 +132,7 @@ def test_get_eslint_config_finds_eslintrc_yaml(base_args): rules: no-console: error""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc.yaml": @@ -157,9 +149,7 @@ def side_effect(file_name, **kwargs): def test_get_eslint_config_finds_eslintrc(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc": @@ -187,9 +177,7 @@ def test_get_eslint_config_finds_eslint_config_js(base_args): } };""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "eslint.config.js": @@ -213,9 +201,7 @@ def test_get_eslint_config_finds_eslint_config_mjs(base_args): } };""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "eslint.config.mjs": @@ -239,9 +225,7 @@ def test_get_eslint_config_finds_eslint_config_cjs(base_args): } };""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "eslint.config.cjs": @@ -268,9 +252,7 @@ def test_get_eslint_config_finds_in_package_json(base_args): } }""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "package.json": @@ -297,9 +279,7 @@ def test_get_eslint_config_package_json_without_eslint_config(base_args): } }""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "package.json": @@ -314,9 +294,7 @@ def side_effect(file_name, **kwargs): def test_get_eslint_config_not_found(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: mock_read.return_value = None result = get_eslint_config(base_args) @@ -325,9 +303,7 @@ def test_get_eslint_config_not_found(base_args): def test_get_eslint_config_priority_order(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == ".eslintrc.js": @@ -345,9 +321,7 @@ def side_effect(file_name, **kwargs): def test_get_eslint_config_tries_all_config_files(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: mock_read.return_value = None result = get_eslint_config(base_args) @@ -359,9 +333,7 @@ def test_get_eslint_config_tries_all_config_files(base_args): def test_get_eslint_config_with_empty_package_json(base_args): package_json_content = "{}" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "package.json": @@ -396,9 +368,7 @@ def test_get_eslint_config_with_complex_eslint_config_in_package_json(base_args) } }""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "package.json": @@ -420,9 +390,7 @@ def side_effect(file_name, **kwargs): def test_get_eslint_config_handles_exception_gracefully(base_args): - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: mock_read.side_effect = Exception("Network error") result = get_eslint_config(base_args) @@ -433,9 +401,7 @@ def test_get_eslint_config_handles_exception_gracefully(base_args): def test_get_eslint_config_handles_json_decode_error(base_args): invalid_json = "{ invalid json content" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "package.json": @@ -451,9 +417,7 @@ def side_effect(file_name, **kwargs): def test_get_eslint_config_first_found_wins(base_args): """Test that flat configs are checked before legacy configs.""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: def side_effect(file_name, **kwargs): if file_name == "eslint.config.mjs": @@ -483,9 +447,7 @@ def test_get_eslint_config_skips_to_package_json_when_no_config_files(base_args) } }""" - with patch( - "services.github.files.get_eslint_config.read_local_file" - ) as mock_read: + with patch("services.github.files.get_eslint_config.read_local_file") as mock_read: call_count = 0 def side_effect(file_name, **kwargs): diff --git a/services/webhook/review_run_handler.py b/services/webhook/review_run_handler.py index 2e016c1a..5a53437e 100644 --- a/services/webhook/review_run_handler.py +++ b/services/webhook/review_run_handler.py @@ -9,7 +9,7 @@ from anthropic.types import MessageParam # Local imports -from config import GITHUB_APP_USER_NAME +from config import GITHUB_APP_USER_NAME, PRODUCT_ID from constants.agent import MAX_ITERATIONS from constants.triggers import ReviewTrigger from payloads.github.pull_request_review_comment.types import ( @@ -71,6 +71,7 @@ async def handle_review_run( comment_author = review["user"] comment_author_type = comment_author["type"] if comment_author_type == "Bot": + logger.info("Ignoring review comment from bot: %s", comment_author["login"]) return # Extract repository related variables @@ -97,13 +98,17 @@ async def handle_review_run( head_branch = pull_request["head"]["ref"] # gitauto/dashboard-20250101-155924-Ab1C base_branch = pull_request["base"]["ref"] # main, master, etc. pr_creator = pull_request["user"]["login"] - if pr_creator != GITHUB_APP_USER_NAME: + if not head_branch.startswith(PRODUCT_ID + "/"): + logger.info( + "Ignoring review comment on non-GitAuto PR (branch: %s)", head_branch + ) return # Extract sender related variables and return if sender is GitAuto itself sender_id: int = payload["sender"]["id"] sender_name: str = payload["sender"]["login"] if sender_name == GITHUB_APP_USER_NAME: + logger.info("Ignoring review comment from GitAuto itself") return # Extract other information diff --git a/services/webhook/test_new_pr_handler.py b/services/webhook/test_new_pr_handler.py index d4a62d24..89a4f9e0 100644 --- a/services/webhook/test_new_pr_handler.py +++ b/services/webhook/test_new_pr_handler.py @@ -463,7 +463,10 @@ async def test_image_base64_fetch_failed( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.get_pull_request_files") @patch("services.webhook.new_pr_handler.chat_with_agent") @patch("services.webhook.new_pr_handler.create_empty_commit") @@ -541,7 +544,10 @@ async def test_timeout_approaching_breaks_loop( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.get_pull_request_files") @patch("services.webhook.new_pr_handler.chat_with_agent") @patch("services.webhook.new_pr_handler.create_empty_commit") @@ -619,7 +625,10 @@ async def test_branch_deleted_breaks_loop( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.MAX_ITERATIONS", 10) @patch("services.webhook.new_pr_handler.verify_task_is_complete") @patch("services.webhook.new_pr_handler.get_pull_request_files") @@ -796,7 +805,10 @@ async def test_retry_loop_exhausted_not_explored_but_committed( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.MAX_ITERATIONS", 9) @patch("services.webhook.new_pr_handler.verify_task_is_complete") @patch("services.webhook.new_pr_handler.get_pull_request_files") @@ -964,7 +976,10 @@ async def test_retry_loop_exhausted_explored_but_not_committed( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.get_pull_request_files") @patch("services.webhook.new_pr_handler.chat_with_agent") @patch("services.webhook.new_pr_handler.create_empty_commit") @@ -1070,7 +1085,10 @@ async def test_retry_counter_reset_on_successful_loop( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.is_test_file") @patch("services.webhook.new_pr_handler.get_pull_request_files") @patch("services.webhook.new_pr_handler.chat_with_agent") @@ -1159,7 +1177,10 @@ async def test_non_test_file_skipped_in_header_merge( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.replace_remote_file_content") @patch("services.webhook.new_pr_handler.merge_test_file_headers") @patch("services.webhook.new_pr_handler.get_raw_content") @@ -1259,7 +1280,10 @@ async def test_test_file_header_merge( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.replace_remote_file_content") @patch("services.webhook.new_pr_handler.merge_test_file_headers") @patch("services.webhook.new_pr_handler.get_raw_content") @@ -1358,7 +1382,10 @@ async def test_test_file_header_merge_no_content( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.replace_remote_file_content") @patch("services.webhook.new_pr_handler.merge_test_file_headers") @patch("services.webhook.new_pr_handler.get_raw_content") @@ -1461,7 +1488,10 @@ async def test_test_file_header_merge_no_change( @patch("services.webhook.new_pr_handler.insert_email_send", return_value=True) @patch("services.webhook.new_pr_handler.send_email") @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.get_credits_depleted_email_text") @patch("services.webhook.new_pr_handler.get_user") @patch("services.webhook.new_pr_handler.get_owner") @@ -1567,7 +1597,10 @@ async def test_credits_depleted_email_sent( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.insert_credit") @patch("services.webhook.new_pr_handler.should_bail", return_value=False) @patch("services.webhook.new_pr_handler.create_empty_commit") @@ -1742,7 +1775,10 @@ async def test_new_pr_handler_token_accumulation( @pytest.mark.asyncio -@patch("services.webhook.new_pr_handler.read_local_file", return_value="def calculate():\n return 1 + 2\n") +@patch( + "services.webhook.new_pr_handler.read_local_file", + return_value="def calculate():\n return 1 + 2\n", +) @patch("services.webhook.new_pr_handler.insert_credit") @patch("services.webhook.new_pr_handler.should_bail", return_value=False) @patch("services.webhook.new_pr_handler.create_empty_commit") @@ -1988,7 +2024,9 @@ async def test_few_test_files_include_contents_in_prompt( "tests/logger.spec.ts", "tests/logger.test.ts", ] - mock_read_local_file.return_value = "function log(msg: string) { console.log(msg); }" + mock_read_local_file.return_value = ( + "function log(msg: string) { console.log(msg); }" + ) mock_chat_with_agent.return_value = AgentResult( messages=[], @@ -2132,7 +2170,9 @@ async def test_many_test_files_include_paths_only_in_prompt( # Return 7 test files (>5 threshold) mock_find_test_files.return_value = [f"tests/test_logger_{i}.ts" for i in range(7)] - mock_read_local_file.return_value = "function log(msg: string) { console.log(msg); }" + mock_read_local_file.return_value = ( + "function log(msg: string) { console.log(msg); }" + ) mock_chat_with_agent.return_value = AgentResult( messages=[], diff --git a/services/webhook/test_review_run_handler.py b/services/webhook/test_review_run_handler.py index d21b38e0..498467f3 100644 --- a/services/webhook/test_review_run_handler.py +++ b/services/webhook/test_review_run_handler.py @@ -7,6 +7,7 @@ import pytest +from config import PRODUCT_ID from services.agents.verify_task_is_complete import VerifyTaskIsCompleteResult from services.chat_with_agent import AgentResult from services.webhook.review_run_handler import handle_review_run @@ -33,7 +34,7 @@ def mock_review_comment_payload(): "body": "This PR adds a new feature to the codebase", "url": "https://api.github.com/repos/test-owner/test-repo/pulls/123", "user": {"login": "gitauto-ai[bot]"}, - "head": {"ref": "feature-branch", "sha": "abc123def456"}, + "head": {"ref": f"{PRODUCT_ID}/dashboard-20250101-155924-Ab1C", "sha": "abc123def456"}, "base": {"ref": "main"}, }, "repository": {