diff --git a/commitloom/cli/cli_handler.py b/commitloom/cli/cli_handler.py index fdeb48e..99e34b2 100644 --- a/commitloom/cli/cli_handler.py +++ b/commitloom/cli/cli_handler.py @@ -83,7 +83,6 @@ def _process_single_commit(self, files: list[GitFile]) -> None: # Print analysis console.print_warnings(analysis) self._maybe_create_branch(analysis) - self._maybe_create_branch(analysis) try: # Generate commit message @@ -239,9 +238,7 @@ def _create_batches(self, changed_files: list[GitFile]) -> list[list[GitFile]]: invalid_files = [] for file in changed_files: - if hasattr(self.git, "should_ignore_file") and self.git.should_ignore_file( - file.path - ): + if self.git.should_ignore_file(file.path): invalid_files.append(file) console.print_warning(f"Ignoring file: {file.path}") else: @@ -289,18 +286,16 @@ def _create_combined_commit(self, batches: list[dict]) -> None: # Create combined commit message title = "📦 chore: combine multiple changes" - body = "\n\n".join( - [ - title, - "\n".join( - f"{data['emoji']} {category}:" for category, data in all_changes.items() - ), - "\n".join( - f"- {change}" for data in all_changes.values() for change in data["changes"] - ), - " ".join(summary_points), - ] - ) + body_parts = [ + "\n".join( + f"{data['emoji']} {category}:" for category, data in all_changes.items() + ), + "\n".join( + f"- {change}" for data in all_changes.values() for change in data["changes"] + ), + " ".join(summary_points), + ] + body = "\n\n".join(part for part in body_parts if part) # Stage and commit all files self.git.stage_files(all_files) diff --git a/commitloom/core/analyzer.py b/commitloom/core/analyzer.py index e99bc1b..11e3140 100644 --- a/commitloom/core/analyzer.py +++ b/commitloom/core/analyzer.py @@ -175,7 +175,7 @@ def format_cost_for_humans(cost: float) -> str: elif cost >= 0.01: return f"{cost*100:.2f}¢" else: - return "0.10¢" # For very small costs, show as 0.10¢ + return f"{cost*100:.2f}¢" @staticmethod def get_cost_context(total_cost: float) -> str: diff --git a/commitloom/core/git.py b/commitloom/core/git.py index 3c2b318..04b8514 100644 --- a/commitloom/core/git.py +++ b/commitloom/core/git.py @@ -4,6 +4,9 @@ import os import subprocess from dataclasses import dataclass +from fnmatch import fnmatch + +from ..config.settings import config logger = logging.getLogger(__name__) @@ -38,6 +41,14 @@ def is_renamed(self) -> bool: class GitOperations: """Basic git operations handler.""" + @staticmethod + def should_ignore_file(path: str) -> bool: + """Check if a file should be ignored based on configured patterns.""" + for pattern in config.ignored_patterns: + if fnmatch(path, pattern): + return True + return False + @staticmethod def _handle_git_output(result: subprocess.CompletedProcess, context: str = "") -> None: """Handle git command output and log messages.""" diff --git a/commitloom/services/ai_service.py b/commitloom/services/ai_service.py index 087886d..6bdf696 100644 --- a/commitloom/services/ai_service.py +++ b/commitloom/services/ai_service.py @@ -3,6 +3,7 @@ import json from dataclasses import dataclass import os +import time import requests @@ -88,6 +89,7 @@ def __init__(self, api_key: str | None = None, test_mode: bool = False): self.test_mode = test_mode # Permitir override por variable de entorno self.model_name = os.getenv("COMMITLOOM_MODEL", config.default_model) + self.session = requests.Session() @property def model(self) -> str: @@ -106,14 +108,15 @@ def token_usage_from_api_usage(cls, usage: dict[str, int]) -> TokenUsage: def generate_prompt(self, diff: str, changed_files: list[GitFile]) -> str: """Generate the prompt for the AI model.""" files_summary = ", ".join(f.path for f in changed_files) + has_binary = any(f.is_binary for f in changed_files) + binary_files = ", ".join(f.path for f in changed_files if f.is_binary) + text_files = [f for f in changed_files if not f.is_binary] - # Check if we're dealing with binary files - if diff.startswith("Binary files changed:"): + if has_binary and not text_files: return ( "Generate a structured commit message for the following binary file changes.\n" "You must respond ONLY with a valid JSON object.\n\n" - f"Files changed: {files_summary}\n\n" - f"{diff}\n\n" + f"Files changed: {binary_files}\n\n" "Requirements:\n" "1. Title: Maximum 50 characters, starting with an appropriate " "gitemoji (📝 for data files), followed by the semantic commit " @@ -128,18 +131,22 @@ def generate_prompt(self, diff: str, changed_files: list[GitFile]) -> str: ' "emoji": "📝",\n' ' "changes": [\n' ' "Updated binary files with new data",\n' - ' "Files affected: example.bin"\n' + f' "Files affected: {binary_files}"\n' " ]\n" " }\n" " },\n" - ' "summary": "Updated binary files with new data"\n' + f' "summary": "Updated binary files: {binary_files}"\n' "}" ) - return ( + prompt = ( "Generate a structured commit message for the following git diff.\n" "You must respond ONLY with a valid JSON object.\n\n" f"Files changed: {files_summary}\n\n" + ) + if binary_files: + prompt += f"Binary files: {binary_files}\n\n" + prompt += ( "```\n" f"{diff}\n" "```\n\n" @@ -172,6 +179,7 @@ def generate_prompt(self, diff: str, changed_files: list[GitFile]) -> str: ' "summary": "Added new feature X with configuration updates"\n' "}" ) + return prompt def generate_commit_message( self, diff: str, changed_files: list[GitFile] @@ -213,36 +221,53 @@ def generate_commit_message( "temperature": 0.7, } - try: - response = requests.post( - "https://api.openai.com/v1/chat/completions", - headers=headers, - json=data, - timeout=30, - ) + last_exception: requests.exceptions.RequestException | None = None + response: requests.Response | None = None + for attempt in range(3): + try: + response = self.session.post( + "https://api.openai.com/v1/chat/completions", + headers=headers, + json=data, + timeout=30, + ) + if response.status_code >= 500: + raise requests.exceptions.RequestException( + f"Server error: {response.status_code}", response=response + ) + break + except requests.exceptions.RequestException as e: + last_exception = e + if attempt == 2: + break + time.sleep(2**attempt) + + if last_exception and (response is None or response.status_code >= 500): + if ( + hasattr(last_exception, "response") + and last_exception.response is not None + and hasattr(last_exception.response, "text") + ): + error_message = last_exception.response.text + else: + error_message = str(last_exception) + raise ValueError(f"API Request failed: {error_message}") from last_exception - if response.status_code == 400: - error_data = response.json() - error_message = error_data.get("error", {}).get("message", "Unknown error") - raise ValueError(f"API Error: {error_message}") + if response.status_code == 400: + error_data = response.json() + error_message = error_data.get("error", {}).get("message", "Unknown error") + raise ValueError(f"API Error: {error_message}") - response.raise_for_status() - response_data = response.json() - content = response_data["choices"][0]["message"]["content"] - usage = response_data["usage"] + response.raise_for_status() + response_data = response.json() + content = response_data["choices"][0]["message"]["content"] + usage = response_data["usage"] - try: - commit_data = json.loads(content) - return CommitSuggestion(**commit_data), TokenUsage.from_api_usage(usage) - except json.JSONDecodeError as e: - raise ValueError(f"Failed to parse AI response: {str(e)}") from e - - except requests.exceptions.RequestException as e: - if hasattr(e, "response") and e.response is not None and hasattr(e.response, "text"): - error_message = e.response.text - else: - error_message = str(e) - raise ValueError(f"API Request failed: {error_message}") from e + try: + commit_data = json.loads(content) + return CommitSuggestion(**commit_data), TokenUsage.from_api_usage(usage) + except json.JSONDecodeError as e: + raise ValueError(f"Failed to parse AI response: {str(e)}") from e @staticmethod def format_commit_message(commit_data: CommitSuggestion) -> str: diff --git a/tests/test_ai_service.py b/tests/test_ai_service.py index c906d49..2850799 100644 --- a/tests/test_ai_service.py +++ b/tests/test_ai_service.py @@ -42,17 +42,26 @@ def test_generate_prompt_text_files(ai_service, mock_git_file): def test_generate_prompt_binary_files(ai_service, mock_git_file): """Test prompt generation for binary files.""" - files = [mock_git_file("image.png", size=1024)] - diff = "Binary files changed" + files = [mock_git_file("image.png", size=1024, hash_="abc123")] + prompt = ai_service.generate_prompt("", files) + assert "image.png" in prompt + assert "binary file changes" in prompt - prompt = ai_service.generate_prompt(diff, files) +def test_generate_prompt_mixed_files(ai_service, mock_git_file): + """Prompt should mention both binary and text changes.""" + files = [ + mock_git_file("image.png", size=1024, hash_="abc123"), + mock_git_file("test.py"), + ] + diff = "diff content" + prompt = ai_service.generate_prompt(diff, files) assert "image.png" in prompt - assert "Binary files changed" in prompt + assert "test.py" in prompt + assert "Binary files" in prompt -@patch("requests.post") -def test_generate_commit_message_success(mock_post, ai_service, mock_git_file): +def test_generate_commit_message_success(ai_service, mock_git_file): """Test successful commit message generation.""" mock_response = { "choices": [ @@ -76,20 +85,25 @@ def test_generate_commit_message_success(mock_post, ai_service, mock_git_file): "usage": {"prompt_tokens": 100, "completion_tokens": 50, "total_tokens": 150}, } - mock_post.return_value = MagicMock(status_code=200, json=lambda: mock_response) + ai_service.session.post = MagicMock( + return_value=MagicMock(status_code=200, json=lambda: mock_response) + ) - suggestion, usage = ai_service.generate_commit_message("test diff", [mock_git_file("test.py")]) + suggestion, usage = ai_service.generate_commit_message( + "test diff", [mock_git_file("test.py")] + ) assert isinstance(suggestion, CommitSuggestion) assert suggestion.title == "✨ feat: add new feature" assert usage.total_tokens == 150 -@patch("requests.post") -def test_generate_commit_message_api_error(mock_post, ai_service, mock_git_file): +def test_generate_commit_message_api_error(ai_service, mock_git_file): """Test handling of API errors.""" - mock_post.return_value = MagicMock( - status_code=400, json=lambda: {"error": {"message": "API Error"}} + ai_service.session.post = MagicMock( + return_value=MagicMock( + status_code=400, json=lambda: {"error": {"message": "API Error"}} + ) ) with pytest.raises(ValueError) as exc_info: @@ -98,15 +112,16 @@ def test_generate_commit_message_api_error(mock_post, ai_service, mock_git_file) assert "API Error" in str(exc_info.value) -@patch("requests.post") -def test_generate_commit_message_invalid_json(mock_post, ai_service, mock_git_file): +def test_generate_commit_message_invalid_json(ai_service, mock_git_file): """Test handling of invalid JSON response.""" mock_response = { "choices": [{"message": {"content": "Invalid JSON"}}], "usage": {"prompt_tokens": 100, "completion_tokens": 50, "total_tokens": 150}, } - mock_post.return_value = MagicMock(status_code=200, json=lambda: mock_response) + ai_service.session.post = MagicMock( + return_value=MagicMock(status_code=200, json=lambda: mock_response) + ) with pytest.raises(ValueError) as exc_info: ai_service.generate_commit_message("test diff", [mock_git_file("test.py")]) @@ -114,10 +129,11 @@ def test_generate_commit_message_invalid_json(mock_post, ai_service, mock_git_fi assert "Failed to parse AI response" in str(exc_info.value) -@patch("requests.post") -def test_generate_commit_message_network_error(mock_post, ai_service, mock_git_file): +def test_generate_commit_message_network_error(ai_service, mock_git_file): """Test handling of network errors.""" - mock_post.side_effect = requests.exceptions.RequestException("Network Error") + ai_service.session.post = MagicMock( + side_effect=requests.exceptions.RequestException("Network Error") + ) with pytest.raises(ValueError) as exc_info: ai_service.generate_commit_message("test diff", [mock_git_file("test.py")]) @@ -125,6 +141,43 @@ def test_generate_commit_message_network_error(mock_post, ai_service, mock_git_f assert "Network Error" in str(exc_info.value) +@patch("time.sleep", return_value=None) +def test_generate_commit_message_retries(mock_sleep, ai_service, mock_git_file): + """Temporary failures should be retried.""" + mock_response = { + "choices": [ + { + "message": { + "content": json.dumps( + { + "title": "✨ feat: retry success", + "body": { + "Features": { + "emoji": "✨", + "changes": ["Added new functionality"], + } + }, + "summary": "Added new feature", + } + ) + } + } + ], + "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2}, + } + ai_service.session.post = MagicMock( + side_effect=[ + requests.exceptions.RequestException("temp"), + MagicMock(status_code=200, json=lambda: mock_response), + ] + ) + suggestion, _ = ai_service.generate_commit_message( + "diff", [mock_git_file("test.py")] + ) + assert suggestion.title == "✨ feat: retry success" + assert ai_service.session.post.call_count == 2 + + def test_format_commit_message(): """Test commit message formatting.""" suggestion = CommitSuggestion( @@ -146,3 +199,17 @@ def test_ai_service_missing_api_key(): AIService(api_key=None) assert "API key is required" in str(exc_info.value) + + +@patch("time.sleep", return_value=None) +def test_generate_commit_message_retries_exhausted( + mock_sleep, ai_service, mock_git_file +): + """Should raise error after exhausting all retries.""" + ai_service.session.post = MagicMock( + side_effect=requests.exceptions.RequestException("temp") + ) + with pytest.raises(ValueError) as exc_info: + ai_service.generate_commit_message("diff", [mock_git_file("test.py")]) + assert "API Request failed" in str(exc_info.value) + assert ai_service.session.post.call_count == 3 diff --git a/tests/test_analyzer.py b/tests/test_analyzer.py index a16860d..b0f3929 100644 --- a/tests/test_analyzer.py +++ b/tests/test_analyzer.py @@ -1,6 +1,5 @@ """Tests for commit analyzer module.""" - import pytest from commitloom.config.settings import config @@ -52,7 +51,9 @@ def test_analyze_diff_complexity_token_limit_exceeded(analyzer, mock_git_file): def test_analyze_diff_complexity_many_files(analyzer, mock_git_file): """Test analysis when many files are changed.""" diff = "Multiple file changes" - files = [mock_git_file(f"file{i}.py") for i in range(config.max_files_threshold + 1)] + files = [ + mock_git_file(f"file{i}.py") for i in range(config.max_files_threshold + 1) + ] analysis = analyzer.analyze_diff_complexity(diff, files) @@ -63,7 +64,9 @@ def test_analyze_diff_complexity_many_files(analyzer, mock_git_file): def test_analyze_diff_complexity_expensive_change(analyzer, mock_git_file): """Test analysis of an expensive change.""" # Create a diff that will be expensive (>0.10€) - tokens_for_10_cents = int((0.10 * 1_000_000) / config.model_costs[config.default_model].input) + tokens_for_10_cents = int( + (0.10 * 1_000_000) / config.model_costs[config.default_model].input + ) diff = "diff --git a/expensive.py b/expensive.py\n" + ( "+" + "x" * tokens_for_10_cents * config.token_estimation_ratio + "\n" ) @@ -115,7 +118,9 @@ def test_analyze_diff_complexity_multiple_conditions(analyzer, mock_git_file): # 1. Many files # 2. Moderate cost # 3. One large file - files = [mock_git_file(f"file{i}.py") for i in range(config.max_files_threshold + 1)] + files = [ + mock_git_file(f"file{i}.py") for i in range(config.max_files_threshold + 1) + ] tokens = config.token_limit * 0.8 diff = "x" * int(tokens * config.token_estimation_ratio) @@ -175,6 +180,7 @@ def test_analyze_diff_complexity_git_format(analyzer, mock_git_file): def test_format_cost_for_humans(): """Test cost formatting.""" + assert CommitAnalyzer.format_cost_for_humans(0.0001) == "0.01¢" assert CommitAnalyzer.format_cost_for_humans(0.001) == "0.10¢" assert CommitAnalyzer.format_cost_for_humans(0.01) == "1.00¢" assert CommitAnalyzer.format_cost_for_humans(0.1) == "10.00¢" @@ -188,3 +194,26 @@ def test_get_cost_context(): assert "moderate" in CommitAnalyzer.get_cost_context(0.05) assert "expensive" in CommitAnalyzer.get_cost_context(0.1) assert "very expensive" in CommitAnalyzer.get_cost_context(1.0) + + +def test_estimate_tokens_and_cost_unknown_model(capsys): + """Fallback to zero cost for unknown model.""" + tokens, cost = CommitAnalyzer.estimate_tokens_and_cost("test", model="unknown") + captured = capsys.readouterr() + assert "Cost estimation is not available" in captured.out + assert tokens >= 0 + assert cost == 0 + + +def test_analyze_diff_complexity_moderate_cost(analyzer, mock_git_file): + """Should warn about moderate cost without marking complex.""" + tokens_for_six_cents = int( + (0.06 * 1_000_000) / config.model_costs[config.default_model].input + ) + diff = "diff --git a/mod.py b/mod.py\n" + ( + "+" + "x" * tokens_for_six_cents * config.token_estimation_ratio + "\n" + ) + files = [mock_git_file("mod.py")] + analysis = analyzer.analyze_diff_complexity(diff, files) + assert any("moderate" in str(w) for w in analysis.warnings) + assert analysis.is_complex diff --git a/tests/test_cli_handler.py b/tests/test_cli_handler.py index 7d92108..e9bfa20 100644 --- a/tests/test_cli_handler.py +++ b/tests/test_cli_handler.py @@ -83,7 +83,10 @@ def test_handle_commit_success(cli): def test_handle_commit_complex_changes(cli): """Test handling complex changes.""" - mock_files = [GitFile(f"test{i}.py", "A", old_path=None, size=100, hash="abc123") for i in range(4)] + mock_files = [ + GitFile(f"test{i}.py", "A", old_path=None, size=100, hash="abc123") + for i in range(4) + ] cli.git.get_staged_files = MagicMock(return_value=mock_files) cli.git.create_commit = MagicMock(return_value=True) @@ -121,7 +124,9 @@ def test_handle_commit_api_error(cli): """Test handling API error.""" mock_file = GitFile("test.py", "A", old_path=None, size=100, hash="abc123") cli.git.get_staged_files = MagicMock(return_value=[mock_file]) - cli.ai_service.generate_commit_message = MagicMock(side_effect=Exception("API error")) + cli.ai_service.generate_commit_message = MagicMock( + side_effect=Exception("API error") + ) with pytest.raises(SystemExit) as exc: cli.run(auto_commit=True) @@ -136,9 +141,6 @@ def test_create_batches_with_ignored_files(cli): GitFile("node_modules/test.js", "A", old_path=None, size=100, hash="def456"), GitFile("test2.py", "A", old_path=None, size=100, hash="ghi789"), ] - cli.git.get_staged_files = MagicMock(return_value=mock_files) - cli.git.should_ignore_file = MagicMock(side_effect=lambda path: "node_modules" in path) - batches = cli._create_batches(mock_files) assert len(batches) == 1 @@ -148,7 +150,9 @@ def test_create_batches_with_ignored_files(cli): def test_create_batches_git_error(cli): """Test batch creation with git error.""" - cli.git.get_staged_files = MagicMock(side_effect=subprocess.CalledProcessError(1, "git")) + cli.git.get_staged_files = MagicMock( + side_effect=subprocess.CalledProcessError(1, "git") + ) batches = cli._create_batches([]) @@ -186,10 +190,11 @@ def test_create_combined_commit_success(cli): }, ] cli.git.create_commit = MagicMock(return_value=True) - cli._create_combined_commit(batches) - cli.git.create_commit.assert_called_once() + args, _ = cli.git.create_commit.call_args + assert args[0] == "📦 chore: combine multiple changes" + assert not args[1].startswith("📦 chore: combine multiple changes") def test_create_combined_commit_no_changes(cli): @@ -224,7 +229,10 @@ def test_debug_mode(cli): def test_process_files_in_batches_error(cli): """Test error handling in batch processing.""" - mock_files = [GitFile(f"test{i}.py", "A", old_path=None, size=100, hash="abc123") for i in range(4)] + mock_files = [ + GitFile(f"test{i}.py", "A", old_path=None, size=100, hash="abc123") + for i in range(4) + ] cli.git.get_diff = MagicMock(side_effect=GitError("Git error")) with pytest.raises(SystemExit) as exc: @@ -283,3 +291,16 @@ def test_maybe_create_branch_not_complex(cli): mock_console.confirm_branch_creation.return_value = True cli._maybe_create_branch(analysis) cli.git.create_and_checkout_branch.assert_not_called() + + +def test_process_single_commit_maybe_create_branch_once(cli, mock_git_file): + """_maybe_create_branch should be invoked only once.""" + cli.auto_commit = True + cli._maybe_create_branch = MagicMock() + cli.git.create_commit = MagicMock(return_value=True) + file = mock_git_file("test.py") + with patch( + "commitloom.cli.cli_handler.metrics_manager.start_commit_tracking" + ), patch("commitloom.cli.cli_handler.metrics_manager.finish_commit_tracking"): + cli._process_single_commit([file]) + cli._maybe_create_branch.assert_called_once() diff --git a/tests/test_git/test_operations.py b/tests/test_git/test_operations.py index 815599d..54a0277 100644 --- a/tests/test_git/test_operations.py +++ b/tests/test_git/test_operations.py @@ -14,6 +14,12 @@ def git_operations(): return GitOperations() +def test_should_ignore_file(git_operations): + """Files matching ignored patterns should be skipped.""" + assert git_operations.should_ignore_file("node_modules/test.js") + assert not git_operations.should_ignore_file("src/app.py") + + @patch("subprocess.run") def test_get_staged_files_success(mock_run, git_operations, mock_git_file): """Test successful retrieval of staged files."""