From 755dc00cc939c34024eacfb557657b75d578d8df Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Thu, 25 Dec 2025 00:33:58 +0900 Subject: [PATCH 1/2] Add check to prevent infinite retry loops in check run failure handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add MAX_GITAUTO_COMMITS_PER_PR constant (50) to prevent infinite loops - Create get_pull_request_commits() to fetch PR commits via GitHub API - Check commit count in check_suite_handler before processing - Post conversational comment on PR when stopping after too many attempts - Add comprehensive tests for get_pull_request_commits() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- constants/general.py | 2 + .../github/pulls/get_pull_request_commits.py | 31 ++++ .../pulls/test_get_pull_request_commits.py | 145 ++++++++++++++++++ services/webhook/check_suite_handler.py | 29 +++- 4 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 services/github/pulls/get_pull_request_commits.py create mode 100644 services/github/pulls/test_get_pull_request_commits.py diff --git a/constants/general.py b/constants/general.py index 34f60870c..23dfe4286 100644 --- a/constants/general.py +++ b/constants/general.py @@ -2,3 +2,5 @@ # https://us-west-1.console.aws.amazon.com/lambda/home?region=us-west-1#/functions/pr-agent-prod?subtab=envVars&tab=configure IS_PRD = get_env_var("ENV") == "prod" + +MAX_GITAUTO_COMMITS_PER_PR = 50 diff --git a/services/github/pulls/get_pull_request_commits.py b/services/github/pulls/get_pull_request_commits.py new file mode 100644 index 000000000..dbaa21d14 --- /dev/null +++ b/services/github/pulls/get_pull_request_commits.py @@ -0,0 +1,31 @@ +# Third party imports +import requests + +# Local imports +from config import GITHUB_API_URL, PER_PAGE, TIMEOUT +from services.github.utils.create_headers import create_headers +from utils.error.handle_exceptions import handle_exceptions + + +@handle_exceptions(default_return_value=[], raise_on_error=False) +def get_pull_request_commits(owner: str, repo: str, pull_number: int, token: str): + url = f"{GITHUB_API_URL}/repos/{owner}/{repo}/pulls/{pull_number}/commits" + headers = create_headers(token=token) + commits = [] + page = 1 + + while True: + params = {"per_page": PER_PAGE, "page": page} + response = requests.get( # pylint: disable=no-member + url=url, headers=headers, params=params, timeout=TIMEOUT + ) + response.raise_for_status() + page_commits = response.json() + + if not page_commits: + break + + commits.extend(page_commits) + page += 1 + + return commits diff --git a/services/github/pulls/test_get_pull_request_commits.py b/services/github/pulls/test_get_pull_request_commits.py new file mode 100644 index 000000000..dded8c603 --- /dev/null +++ b/services/github/pulls/test_get_pull_request_commits.py @@ -0,0 +1,145 @@ +# Standard imports +from unittest.mock import MagicMock, patch + +# Third party imports +import requests + +# Local imports +from services.github.pulls.get_pull_request_commits import get_pull_request_commits + + +def test_get_pull_request_commits_success_single_page(): + mock_commits = [ + { + "sha": "abc123", + "commit": { + "author": {"name": "Test User", "email": "test@example.com"}, + "message": "Test commit 1", + }, + }, + { + "sha": "def456", + "commit": { + "author": { + "name": "gitauto-ai[bot]", + "email": "161652217+gitauto-ai[bot]@users.noreply.github.com", + }, + "message": "Test commit 2", + }, + }, + ] + + with patch( + "services.github.pulls.get_pull_request_commits.requests.get" + ) as mock_get, patch( + "services.github.pulls.get_pull_request_commits.create_headers" + ) as mock_headers: + + mock_response = MagicMock() + mock_response.json.return_value = mock_commits + mock_get.return_value = mock_response + mock_headers.return_value = {"Authorization": "Bearer test_token"} + + result = get_pull_request_commits("owner", "repo", 123, "test_token") + + mock_get.assert_called_once_with( + url="https://api.github.com/repos/owner/repo/pulls/123/commits", + headers={"Authorization": "Bearer test_token"}, + params={"per_page": 100, "page": 1}, + timeout=120, + ) + mock_response.raise_for_status.assert_called_once() + + assert result == mock_commits + assert len(result) == 2 + + +def test_get_pull_request_commits_success_multiple_pages(): + page1_commits = [{"sha": f"commit{i}"} for i in range(100)] + page2_commits = [{"sha": f"commit{i}"} for i in range(100, 150)] + + with patch( + "services.github.pulls.get_pull_request_commits.requests.get" + ) as mock_get, patch( + "services.github.pulls.get_pull_request_commits.create_headers" + ) as mock_headers: + + mock_response_page1 = MagicMock() + mock_response_page1.json.return_value = page1_commits + mock_response_page2 = MagicMock() + mock_response_page2.json.return_value = page2_commits + mock_response_page3 = MagicMock() + mock_response_page3.json.return_value = [] + + mock_get.side_effect = [ + mock_response_page1, + mock_response_page2, + mock_response_page3, + ] + mock_headers.return_value = {"Authorization": "Bearer test_token"} + + result = get_pull_request_commits("owner", "repo", 456, "test_token") + + assert mock_get.call_count == 3 + assert len(result) == 150 + assert result[0] == {"sha": "commit0"} + assert result[99] == {"sha": "commit99"} + assert result[100] == {"sha": "commit100"} + + +def test_get_pull_request_commits_empty_result(): + with patch( + "services.github.pulls.get_pull_request_commits.requests.get" + ) as mock_get, patch( + "services.github.pulls.get_pull_request_commits.create_headers" + ) as mock_headers: + + mock_response = MagicMock() + mock_response.json.return_value = [] + mock_get.return_value = mock_response + mock_headers.return_value = {"Authorization": "Bearer test_token"} + + result = get_pull_request_commits("owner", "repo", 123, "test_token") + + assert not result + assert len(result) == 0 + + +def test_get_pull_request_commits_http_error(): + with patch( + "services.github.pulls.get_pull_request_commits.requests.get" + ) as mock_get, patch( + "services.github.pulls.get_pull_request_commits.create_headers" + ) as mock_headers: + + mock_response = MagicMock() + mock_response.status_code = 404 + http_error = requests.exceptions.HTTPError( # pylint: disable=no-member + "404 Client Error" + ) + http_error.response = mock_response + mock_response.raise_for_status.side_effect = http_error + + mock_get.return_value = mock_response + mock_headers.return_value = {"Authorization": "Bearer test_token"} + + result = get_pull_request_commits("owner", "repo", 999, "test_token") + + assert not result + + +def test_get_pull_request_commits_network_error(): + with patch( + "services.github.pulls.get_pull_request_commits.requests.get" + ) as mock_get, patch( + "services.github.pulls.get_pull_request_commits.create_headers" + ) as mock_headers: + + mock_get.side_effect = requests.exceptions.ConnectionError( # pylint: disable=no-member + "Network error" + ) + mock_headers.return_value = {"Authorization": "Bearer test_token"} + + result = get_pull_request_commits("owner", "repo", 123, "test_token") + + assert not result diff --git a/services/webhook/check_suite_handler.py b/services/webhook/check_suite_handler.py index da6e3369e..777b12805 100644 --- a/services/webhook/check_suite_handler.py +++ b/services/webhook/check_suite_handler.py @@ -5,7 +5,8 @@ import time # Local imports -from config import EMAIL_LINK, PRODUCT_ID, UTF8 +from config import EMAIL_LINK, GITHUB_APP_USER_NAME, PRODUCT_ID, UTF8 +from constants.general import MAX_GITAUTO_COMMITS_PER_PR from constants.messages import PERMISSION_DENIED_MESSAGE, CHECK_RUN_STUMBLED_MESSAGE from services.chat_with_agent import chat_with_agent @@ -26,6 +27,7 @@ get_installation_permissions, ) from services.github.pulls.get_pull_request import get_pull_request +from services.github.pulls.get_pull_request_commits import get_pull_request_commits from services.github.pulls.get_pull_request_files import get_pull_request_files from services.github.pulls.is_pull_request_open import is_pull_request_open from services.github.types.github_types import BaseArgs, CheckSuiteCompletedPayload @@ -217,6 +219,31 @@ def handle_check_suite( slack_notify(msg, thread_ts) return + # Check if there are too many GitAuto commits (prevent infinite retry loops) + pr_commits = get_pull_request_commits( + owner=owner_name, repo=repo_name, pull_number=pull_number, token=token + ) + gitauto_commit_count = 0 + for commit in pr_commits: + commit_author = commit.get("commit", {}).get("author", {}) + commit_name = commit_author.get("name", "") + if GITHUB_APP_USER_NAME in commit_name: + gitauto_commit_count += 1 + + if gitauto_commit_count >= MAX_GITAUTO_COMMITS_PER_PR: + comment_msg = f"I've made {gitauto_commit_count} commits trying to fix this, but the tests keep failing with slightly different errors. I'm going to stop here to avoid an infinite loop. Could you take a look?" + log_msg = f"Stopped after {gitauto_commit_count} commits in PR #{pull_number} in `{owner_name}/{repo_name}` - preventing infinite loop" + print(log_msg) + create_comment( + owner=owner_name, + repo=repo_name, + token=token, + issue_number=pull_number, + body=comment_msg, + ) + slack_notify(log_msg, thread_ts) + return + # Create the first comment p = 0 log_messages = [] From a2ac9a1853e1bfb1fdecb12e8886fc70607a2b2b Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Thu, 25 Dec 2025 01:13:41 +0900 Subject: [PATCH 2/2] Fix infinite loop in test_get_pull_request_commits tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests were hanging because the function uses a while True loop that fetches pages until an empty response is received. The mocks were returning the same non-empty response indefinitely, causing an infinite loop. Fixed by using side_effect to return commits on first call and empty list on second call to properly break the pagination loop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../pulls/test_get_pull_request_commits.py | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/services/github/pulls/test_get_pull_request_commits.py b/services/github/pulls/test_get_pull_request_commits.py index dded8c603..b484a08fd 100644 --- a/services/github/pulls/test_get_pull_request_commits.py +++ b/services/github/pulls/test_get_pull_request_commits.py @@ -35,23 +35,19 @@ def test_get_pull_request_commits_success_single_page(): "services.github.pulls.get_pull_request_commits.create_headers" ) as mock_headers: - mock_response = MagicMock() - mock_response.json.return_value = mock_commits - mock_get.return_value = mock_response + mock_response_page1 = MagicMock() + mock_response_page1.json.return_value = mock_commits + mock_response_page2 = MagicMock() + mock_response_page2.json.return_value = [] + + mock_get.side_effect = [mock_response_page1, mock_response_page2] mock_headers.return_value = {"Authorization": "Bearer test_token"} result = get_pull_request_commits("owner", "repo", 123, "test_token") - mock_get.assert_called_once_with( - url="https://api.github.com/repos/owner/repo/pulls/123/commits", - headers={"Authorization": "Bearer test_token"}, - params={"per_page": 100, "page": 1}, - timeout=120, - ) - mock_response.raise_for_status.assert_called_once() - assert result == mock_commits assert len(result) == 2 + assert mock_get.call_count == 2 def test_get_pull_request_commits_success_multiple_pages(): @@ -103,6 +99,7 @@ def test_get_pull_request_commits_empty_result(): assert not result assert len(result) == 0 + mock_get.assert_called_once() def test_get_pull_request_commits_http_error(): @@ -135,8 +132,10 @@ def test_get_pull_request_commits_network_error(): "services.github.pulls.get_pull_request_commits.create_headers" ) as mock_headers: - mock_get.side_effect = requests.exceptions.ConnectionError( # pylint: disable=no-member - "Network error" + mock_get.side_effect = ( + requests.exceptions.ConnectionError( # pylint: disable=no-member + "Network error" + ) ) mock_headers.return_value = {"Authorization": "Bearer test_token"}