From cf8dec9abf04c2c80800fab5033e78db206c8a3b Mon Sep 17 00:00:00 2001 From: cabana8471 Date: Tue, 27 Jan 2026 06:58:56 +0100 Subject: [PATCH] fix: address CodeRabbit review - extract rate limit logic to shared module - Create rate_limit_utils.py with shared constants and functions - Update agent.py to import from shared module - Update test_agent.py to import from shared module (removes duplication) Co-Authored-By: Claude Opus 4.5 --- agent.py | 88 ++++++++++++++++++--------------------------- rate_limit_utils.py | 69 +++++++++++++++++++++++++++++++++++ test_agent.py | 53 ++++----------------------- 3 files changed, 110 insertions(+), 100 deletions(-) create mode 100644 rate_limit_utils.py diff --git a/agent.py b/agent.py index 3b1bf63..46f1f34 100644 --- a/agent.py +++ b/agent.py @@ -23,7 +23,13 @@ if sys.platform == "win32": sys.stderr = io.TextIOWrapper(sys.stderr.buffer, encoding="utf-8", errors="replace", line_buffering=True) from client import create_client -from progress import count_passing_tests, has_features, print_progress_summary, print_session_header +from progress import ( + clear_stuck_features, + count_passing_tests, + has_features, + print_progress_summary, + print_session_header, +) from prompts import ( copy_spec_to_project, get_coding_prompt, @@ -31,63 +37,15 @@ from prompts import ( get_single_feature_prompt, get_testing_prompt, ) +from rate_limit_utils import ( + RATE_LIMIT_PATTERNS, + is_rate_limit_error, + parse_retry_after, +) # Configuration AUTO_CONTINUE_DELAY_SECONDS = 3 -# Rate limit detection patterns (used in both exception messages and response text) -RATE_LIMIT_PATTERNS = [ - "limit reached", - "rate limit", - "rate_limit", - "too many requests", - "quota exceeded", - "please wait", - "try again later", - "429", - "overloaded", -] - - -def parse_retry_after(error_message: str) -> Optional[int]: - """ - Extract retry-after seconds from various error message formats. - - Returns seconds to wait, or None if not parseable. - """ - # Common patterns: - # "retry after 60 seconds" - # "Retry-After: 120" - # "try again in 5 seconds" - # "30 seconds remaining" - - patterns = [ - r"retry.?after[:\s]+(\d+)\s*(?:seconds?)?", - r"try again in\s+(\d+)\s*(?:seconds?|s\b)", - r"(\d+)\s*seconds?\s*(?:remaining|left|until)", - ] - - for pattern in patterns: - match = re.search(pattern, error_message, re.IGNORECASE) - if match: - return int(match.group(1)) - - return None - - -def is_rate_limit_error(error_message: str) -> bool: - """ - Detect if an error message indicates a rate limit. - - Args: - error_message: The error message to check - - Returns: - True if the error appears to be rate-limit related - """ - error_lower = error_message.lower() - return any(pattern in error_lower for pattern in RATE_LIMIT_PATTERNS) - async def run_agent_session( client: ClaudeSDKClient, @@ -215,6 +173,28 @@ async def run_autonomous_agent( # Create project directory project_dir.mkdir(parents=True, exist_ok=True) + # IMPORTANT: Do NOT clear stuck features in parallel mode! + # The orchestrator manages feature claiming atomically. + # Clearing here causes race conditions where features are marked in_progress + # by the orchestrator but immediately cleared by the agent subprocess on startup. + # + # For single-agent mode or manual runs, clearing is still safe because + # there's only one agent at a time and it happens before claiming any features. + # + # Only clear if we're NOT in a parallel orchestrator context + # (detected by checking if this agent is a subprocess spawned by orchestrator) + import psutil + try: + parent_process = psutil.Process().parent() + parent_name = parent_process.name() if parent_process else "" + + # Only clear if parent is NOT python (i.e., we're running manually, not from orchestrator) + if "python" not in parent_name.lower(): + clear_stuck_features(project_dir) + except Exception: + # If parent process check fails, err on the safe side and clear + clear_stuck_features(project_dir) + # Determine agent type if not explicitly set if agent_type is None: # Auto-detect based on whether we have features diff --git a/rate_limit_utils.py b/rate_limit_utils.py new file mode 100644 index 0000000..6d817f3 --- /dev/null +++ b/rate_limit_utils.py @@ -0,0 +1,69 @@ +""" +Rate Limit Utilities +==================== + +Shared utilities for detecting and handling API rate limits. +Used by both agent.py (production) and test_agent.py (tests). +""" + +import re +from typing import Optional + +# Rate limit detection patterns (used in both exception messages and response text) +RATE_LIMIT_PATTERNS = [ + "limit reached", + "rate limit", + "rate_limit", + "too many requests", + "quota exceeded", + "please wait", + "try again later", + "429", + "overloaded", +] + + +def parse_retry_after(error_message: str) -> Optional[int]: + """ + Extract retry-after seconds from various error message formats. + + Handles common formats: + - "Retry-After: 60" + - "retry after 60 seconds" + - "try again in 5 seconds" + - "30 seconds remaining" + + Args: + error_message: The error message to parse + + Returns: + Seconds to wait, or None if not parseable. + """ + patterns = [ + r"retry.?after[:\s]+(\d+)\s*(?:seconds?)?", + r"try again in\s+(\d+)\s*(?:seconds?|s\b)", + r"(\d+)\s*seconds?\s*(?:remaining|left|until)", + ] + + for pattern in patterns: + match = re.search(pattern, error_message, re.IGNORECASE) + if match: + return int(match.group(1)) + + return None + + +def is_rate_limit_error(error_message: str) -> bool: + """ + Detect if an error message indicates a rate limit. + + Checks against common rate limit patterns from various API providers. + + Args: + error_message: The error message to check + + Returns: + True if the message indicates a rate limit, False otherwise. + """ + error_lower = error_message.lower() + return any(pattern in error_lower for pattern in RATE_LIMIT_PATTERNS) diff --git a/test_agent.py b/test_agent.py index bac4fd7..2af56d5 100644 --- a/test_agent.py +++ b/test_agent.py @@ -1,56 +1,17 @@ """ -Unit tests for agent.py rate limit handling functions. +Unit tests for rate limit handling functions. Tests the parse_retry_after() and is_rate_limit_error() functions -added for improved rate limit handling (Issue #41). +from rate_limit_utils.py (shared module). """ -import re import unittest -from typing import Optional -# Copy the constants and functions from agent.py for isolated testing -# (Avoids dependency on claude_agent_sdk which may not be installed) - -RATE_LIMIT_PATTERNS = [ - "limit reached", - "rate limit", - "rate_limit", - "too many requests", - "quota exceeded", - "please wait", - "try again later", - "429", - "overloaded", -] - - -def parse_retry_after(error_message: str) -> Optional[int]: - """ - Extract retry-after seconds from various error message formats. - - Returns seconds to wait, or None if not parseable. - """ - patterns = [ - r"retry.?after[:\s]+(\d+)\s*(?:seconds?)?", - r"try again in\s+(\d+)\s*(?:seconds?|s\b)", - r"(\d+)\s*seconds?\s*(?:remaining|left|until)", - ] - - for pattern in patterns: - match = re.search(pattern, error_message, re.IGNORECASE) - if match: - return int(match.group(1)) - - return None - - -def is_rate_limit_error(error_message: str) -> bool: - """ - Detect if an error message indicates a rate limit. - """ - error_lower = error_message.lower() - return any(pattern in error_lower for pattern in RATE_LIMIT_PATTERNS) +from rate_limit_utils import ( + RATE_LIMIT_PATTERNS, + is_rate_limit_error, + parse_retry_after, +) class TestParseRetryAfter(unittest.TestCase):