diff --git a/apps/backend/core/error_utils.py b/apps/backend/core/error_utils.py index 98a8fa94..28c1b155 100644 --- a/apps/backend/core/error_utils.py +++ b/apps/backend/core/error_utils.py @@ -114,5 +114,7 @@ def is_authentication_error(error: Exception) -> bool: "token_expired", "not authenticated", "http 401", + "does not have access to claude", + "please login again", ] ) diff --git a/apps/backend/runners/github/services/sdk_utils.py b/apps/backend/runners/github/services/sdk_utils.py index 79d11501..1c277f76 100644 --- a/apps/backend/runners/github/services/sdk_utils.py +++ b/apps/backend/runners/github/services/sdk_utils.py @@ -133,6 +133,52 @@ def _get_tool_detail(tool_name: str, tool_input: dict[str, Any]) -> str: # Prevents runaway retry loops from consuming unbounded resources MAX_MESSAGE_COUNT = 500 +# Abort after 1 consecutive repeat (2 total identical responses). +# Low threshold catches error loops quickly (e.g., auth errors returned as AI text). +# Normal AI responses never produce the exact same text block twice in a row. +REPEATED_RESPONSE_THRESHOLD = 1 + +# Max length for auth error detection - real auth errors are short (~1-2 sentences). +# Longer texts are likely AI discussion about auth topics, not actual errors. +MAX_AUTH_ERROR_LENGTH = 300 + + +def _is_auth_error_response(text: str) -> bool: + """ + Detect authentication/access error messages returned as AI response text. + + Some API errors are returned as conversational text rather than HTTP errors, + causing the SDK to treat them as normal assistant responses. This leads to + infinite retry loops as the conversation ping-pongs between prompts and + error responses. + + Real auth error responses are short messages (~1-2 sentences). AI discussion + text that merely mentions auth topics (e.g., PR reviews about auth features) + is much longer. We skip texts over MAX_AUTH_ERROR_LENGTH chars to avoid + false positives. + + Args: + text: AI response text to check + + Returns: + True if the text is an auth/access error, False otherwise + """ + text_lower = text.lower().strip() + # Real auth error responses are short messages, not long AI discussions. + # Skip texts longer than MAX_AUTH_ERROR_LENGTH to avoid false positives + # when AI discusses authentication topics (e.g., reviewing a PR about auth). + if len(text_lower) > MAX_AUTH_ERROR_LENGTH: + return False + auth_error_patterns = [ + "please login again", + # Catches both "does not have access to claude" and partial variants. + # "account does not have access" was intentionally excluded — it's too + # broad and can match short AI responses about access control generally. + # Generic error loops are caught by REPEATED_RESPONSE_THRESHOLD instead. + "not have access to claude", + ] + return any(pattern in text_lower for pattern in auth_error_patterns) + def _is_tool_concurrency_error(text: str) -> bool: """ @@ -226,6 +272,9 @@ async def process_sdk_stream( completed_agent_tool_ids: set[str] = set() # tool_ids of completed agents # Track tool concurrency errors for retry logic detected_concurrency_error = False + # Track repeated identical responses to detect error loops early + last_response_text: str | None = None + repeated_response_count = 0 # Circuit breaker: max messages before aborting message_limit = max_messages if max_messages is not None else MAX_MESSAGE_COUNT @@ -244,6 +293,10 @@ async def process_sdk_stream( msg_type = type(msg).__name__ msg_count += 1 + # Check if a previous iteration set stream_error (e.g., auth error in text block) + if stream_error: + break + # CIRCUIT BREAKER: Abort if message count exceeds threshold # This prevents runaway retry loops (e.g., 400 errors causing infinite retries) if msg_count > message_limit: @@ -428,6 +481,40 @@ async def process_sdk_stream( block_type = type(block).__name__ if block_type == "TextBlock" and hasattr(block, "text"): result_text += block.text + # Check for auth/access error returned as AI response text. + # Note: break exits this inner for-loop over msg.content; + # the outer message loop exits via `if stream_error: break`. + if _is_auth_error_response(block.text): + stream_error = ( + f"Authentication error detected in AI response: " + f"{block.text[:200].strip()}" + ) + logger.error(f"[{context_name}] {stream_error}") + safe_print(f"[{context_name}] ERROR: {stream_error}") + break + # Check for repeated identical responses (error loop detection). + # Skip empty text blocks so they don't reset the counter. + _stripped = block.text.strip() + if _stripped: + if _stripped == last_response_text: + repeated_response_count += 1 + if ( + repeated_response_count + >= REPEATED_RESPONSE_THRESHOLD + ): + stream_error = ( + f"Repeated response loop detected: same response " + f"received {repeated_response_count + 1} times in a row. " + f"Response: {_stripped[:200]}" + ) + logger.error(f"[{context_name}] {stream_error}") + safe_print( + f"[{context_name}] ERROR: {stream_error}" + ) + break + else: + last_response_text = _stripped + repeated_response_count = 0 # Check for tool concurrency error pattern in text output if _is_tool_concurrency_error(block.text): detected_concurrency_error = True diff --git a/apps/frontend/src/main/__tests__/rate-limit-detector.test.ts b/apps/frontend/src/main/__tests__/rate-limit-detector.test.ts index 32b6cf27..6622a583 100644 --- a/apps/frontend/src/main/__tests__/rate-limit-detector.test.ts +++ b/apps/frontend/src/main/__tests__/rate-limit-detector.test.ts @@ -607,6 +607,20 @@ Please authenticate and try again.`; expect(result.isAuthFailure).toBe(true); }); + + it('should NOT false-positive on AI discussion text mentioning auth topics', async () => { + const { detectAuthFailure } = await import('../rate-limit-detector'); + + // This simulates an AI PR review that discusses authentication — it should NOT trigger auth detection + const aiReviewText = `The PR adds authentication error detection to prevent infinite retry loops. ` + + `When the API returns a message like "does not have access to Claude", the system now detects it. ` + + `However, this pattern could also match if a user discusses authentication in a PR review. ` + + `We should ensure the detection is specific enough to avoid false positives. ` + + `Please login again is another phrase that could appear in normal discussion.`; + const result = detectAuthFailure(aiReviewText); + + expect(result.isAuthFailure).toBe(false); + }); }); }); diff --git a/tests/test_error_utils.py b/tests/test_error_utils.py index 7b2bb8b8..9000f840 100644 --- a/tests/test_error_utils.py +++ b/tests/test_error_utils.py @@ -202,3 +202,106 @@ class TestIsAuthenticationError: def test_case_insensitive(self): err = Exception("UNAUTHORIZED access denied") assert is_authentication_error(err) is True + + def test_does_not_have_access_to_claude(self): + """Detect 'does not have access to Claude' - returned as AI text response.""" + err = Exception( + "Your account does not have access to Claude. " + "Please login again or contact your administrator." + ) + assert is_authentication_error(err) is True + + def test_please_login_again(self): + err = Exception("Please login again to continue.") + assert is_authentication_error(err) is True + + +# ============================================================================= +# _is_auth_error_response (from sdk_utils) +# ============================================================================= + + +class TestIsAuthErrorResponse: + """Tests for _is_auth_error_response() length guard in sdk_utils. + + Uses importlib to load the module directly to avoid heavy package imports. + """ + + @staticmethod + def _load_fn(): + """Load _is_auth_error_response without triggering runners.github.__init__.""" + import importlib.util + import os + + spec = importlib.util.spec_from_file_location( + "sdk_utils", + os.path.join( + os.path.dirname(__file__), + "..", + "apps", + "backend", + "runners", + "github", + "services", + "sdk_utils.py", + ), + ) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod._is_auth_error_response + + def test_short_auth_error_detected(self): + """Short auth error text should be detected.""" + fn = self._load_fn() + assert fn("Your account does not have access to Claude.") is True + + def test_short_please_login_again(self): + """Short 'please login again' text should be detected.""" + fn = self._load_fn() + assert fn("Please login again to continue.") is True + + def test_long_ai_discussion_not_detected(self): + """Long AI discussion text mentioning auth phrases should NOT be detected.""" + fn = self._load_fn() + long_review = ( + "This PR adds authentication error detection to prevent infinite retry loops. " + "When the API returns a message like 'does not have access to Claude', the system " + "now detects it and stops retrying. However, this pattern could also match if a " + "user discusses authentication in a PR review. We should ensure the detection is " + "specific enough to avoid false positives. The phrase 'please login again' could " + "appear in normal discussion about auth flows without indicating an actual error." + ) + assert len(long_review) > 300 + assert fn(long_review) is False + + def test_empty_text_not_detected(self): + """Empty text should not be detected.""" + fn = self._load_fn() + assert fn("") is False + + def test_unrelated_short_text_not_detected(self): + """Short text without auth phrases should not be detected.""" + fn = self._load_fn() + assert fn("Task completed successfully.") is False + + def test_generic_access_denied_not_detected(self): + """Generic 'account does not have access' should NOT trigger (too broad).""" + fn = self._load_fn() + assert fn("This account does not have access to the repository.") is False + assert fn("The service account does not have access to deploy.") is False + + def test_boundary_exactly_300_chars_detected(self): + """Text of exactly 300 chars with auth phrase should be detected.""" + fn = self._load_fn() + base = "does not have access to claude" # 30 chars + text_300 = base + "x" * (300 - len(base)) + assert len(text_300) == 300 + assert fn(text_300) is True + + def test_boundary_301_chars_not_detected(self): + """Text of 301 chars with auth phrase should NOT be detected (> 300).""" + fn = self._load_fn() + base = "does not have access to claude" # 30 chars + text_301 = base + "x" * (301 - len(base)) + assert len(text_301) == 301 + assert fn(text_301) is False