diff --git a/aider_gitea/__init__.py b/aider_gitea/__init__.py index 9fe3f23..ea50c40 100644 --- a/aider_gitea/__init__.py +++ b/aider_gitea/__init__.py @@ -145,19 +145,77 @@ from pathlib import Path from . import secrets from ._version import __version__ # noqa: F401 -from .seen_issues_db import SeenIssuesDB +from .seen_issues_db import SeenIssuesDB as SeenIssuesDB logger = logging.getLogger(__name__) +# Constants for common configuration values +DEFAULT_TIMEOUT_SECONDS = 10_000 +DEFAULT_DAEMON_INTERVAL_SECONDS = 300 +DEFAULT_BASE_BRANCH = 'main' +DEFAULT_PR_CONTEXT_LINES = 3 +DEFAULT_LOG_TAIL_LINES = 100 +DEFAULT_MINIMUM_PYTHON_VERSION = '3.9' + +# Git and repository constants +INITIAL_RUFF_COMMIT_MESSAGE = 'Initial ruff pass' +RUFF_CLEANUP_COMMIT_MESSAGE = 'Ruff after' +PIPELINE_RESOLUTION_COMMIT_PREFIX = 'Resolve pipeline' +COMMENT_RESOLUTION_COMMIT_PREFIX = 'Resolve comment' + +# API and model configuration constants +DEFAULT_ANTHROPIC_INDICATORS = [ + 'claude', + 'anthropic', + 'sonnet', + 'haiku', + 'opus', +] + +# Error messages as constants +ERROR_GITEA_URL_EMPTY = 'gitea_url cannot be empty' +ERROR_OWNER_EMPTY = 'owner cannot be empty' +ERROR_REPO_EMPTY = 'repo cannot be empty' +ERROR_BASE_BRANCH_EMPTY = 'base_branch cannot be empty' +ERROR_GITEA_URL_API_SUFFIX = "gitea_url should not include '/api/v1' suffix" +ERROR_ISSUE_PARAMS_TYPE = 'Both issue_number and issue_title must be strings' +ERROR_ISSUE_NUMBER_EMPTY = 'Issue number cannot be empty' +ERROR_MODEL_TYPE = 'Model must be a string, got {}' + @dataclasses.dataclass(frozen=True) class RepositoryConfig: + """Configuration for a repository to process issues from. + + Attributes: + gitea_url: Base URL for the Gitea instance (without '/api/v1'). + owner: Owner/organization name of the repository. + repo: Repository name. + base_branch: Base branch name to create feature branches from. + """ + gitea_url: str owner: str repo: str base_branch: str + def __post_init__(self): + """Validate repository configuration fields.""" + if not self.gitea_url or not self.gitea_url.strip(): + raise ValueError(ERROR_GITEA_URL_EMPTY) + if not self.owner or not self.owner.strip(): + raise ValueError(ERROR_OWNER_EMPTY) + if not self.repo or not self.repo.strip(): + raise ValueError(ERROR_REPO_EMPTY) + if not self.base_branch or not self.base_branch.strip(): + raise ValueError(ERROR_BASE_BRANCH_EMPTY) + + # Ensure gitea_url doesn't end with /api/v1 (common mistake) + if self.gitea_url.rstrip('/').endswith('/api/v1'): + raise ValueError(ERROR_GITEA_URL_API_SUFFIX) + def repo_url(self) -> str: + """Generate the git clone URL for this repository.""" return f'{self.gitea_url}:{self.owner}/{self.repo}.git'.replace( 'https://', 'git@', @@ -187,8 +245,20 @@ def generate_branch_name(issue_number: str, issue_title: str) -> str: Returns: A sanitized branch name combining the issue number and title. + + Raises: + ValueError: If issue_number is empty or invalid. + TypeError: If arguments are not strings. """ - sanitized = re.sub(r'[^0-9a-zA-Z ]+', '', issue_title) + if not isinstance(issue_number, str) or not isinstance(issue_title, str): + raise TypeError(ERROR_ISSUE_PARAMS_TYPE) + + if not issue_number.strip(): + raise ValueError(ERROR_ISSUE_NUMBER_EMPTY) + + # Sanitize the title, handling empty titles gracefully + sanitized_title = issue_title.strip() if issue_title else 'untitled' + sanitized = re.sub(r'[^0-9a-zA-Z ]+', '', sanitized_title) parts = ['issue', str(issue_number), *sanitized.lower().split()] return '-'.join(parts) @@ -262,7 +332,7 @@ def run_post_solver_cleanup(repository_path: Path, solver_name: str) -> None: run_cmd(['bash', '-c', RUFF_FORMAT_AND_AUTO_FIX], repository_path, check=False) run_cmd(['git', 'add', '.'], repository_path) run_cmd( - ['git', 'commit', '-m', f'Ruff after {solver_name}'], + ['git', 'commit', '-m', f'{RUFF_CLEANUP_COMMIT_MESSAGE} {solver_name}'], repository_path, check=False, ) @@ -306,7 +376,7 @@ class AiderCodeSolver(CodeSolverStrategy): '--yes', '--disable-playwright', '--timeout', - str(10_000), + str(DEFAULT_TIMEOUT_SECONDS), ] if edit_format := MODEL_EDIT_MODES.get(CODE_MODEL): @@ -407,17 +477,27 @@ class ClaudeCodeSolver(CodeSolverStrategy): def is_anthropic_model(model: str) -> bool: - """Check if the model string indicates an Anthropic/Claude model.""" - if not model: + """Check if the model string indicates an Anthropic/Claude model. + + Args: + model: The model name/identifier to check. + + Returns: + True if the model appears to be an Anthropic model, False otherwise. + + Raises: + TypeError: If model is not a string. + """ + if model is None: return False - anthropic_indicators = [ - 'claude', - 'anthropic', - 'sonnet', - 'haiku', - 'opus', - ] + if not isinstance(model, str): + raise TypeError(ERROR_MODEL_TYPE.format(type(model))) + + if not model.strip(): + return False + + anthropic_indicators = DEFAULT_ANTHROPIC_INDICATORS model_lower = model.lower() return any(indicator in model_lower for indicator in anthropic_indicators) @@ -659,7 +739,11 @@ def solve_issue_in_repository( # Run initial ruff pass before code solver run_cmd(['bash', '-c', RUFF_FORMAT_AND_AUTO_FIX], repository_path, check=False) run_cmd(['git', 'add', '.'], repository_path) - run_cmd(['git', 'commit', '-m', 'Initial ruff pass'], repository_path, check=False) + run_cmd( + ['git', 'commit', '-m', INITIAL_RUFF_COMMIT_MESSAGE], + repository_path, + check=False, + ) # Run code solver issue_content = f'# {issue_title}\n{issue_description}' @@ -801,14 +885,26 @@ def handle_pr_comments( for comment in comments: path = comment.get('path') line = comment.get('line') or comment.get('position') or 0 + + if not path: + logger.warning('Comment has no path, skipping') + continue + file_path = repository_path / path try: - lines = file_path.read_text().splitlines() - start = max(0, line - 3) - end = min(len(lines), line + 2) + lines = file_path.read_text(encoding='utf-8').splitlines() + start = max(0, line - DEFAULT_PR_CONTEXT_LINES) + end = min(len(lines), line + DEFAULT_PR_CONTEXT_LINES - 1) context = '\n'.join(lines[start:end]) - except Exception: - context = '' + except FileNotFoundError: + logger.warning('File %s not found for comment context', path) + context = f'File {path} not found' + except UnicodeDecodeError as e: + logger.warning('Failed to decode file %s: %s', path, e) + context = f'Unable to read file {path} (encoding issue)' + except Exception as e: + logger.warning('Failed to read file %s for comment context: %s', path, e) + context = f'Error reading file {path}' body = comment.get('body', '') issue = ( f'Resolve the following reviewer comment:\n{body}\n\n' @@ -819,7 +915,12 @@ def handle_pr_comments( # commit and push changes for this comment run_cmd(['git', 'add', path], repository_path, check=False) run_cmd( - ['git', 'commit', '-m', f'Resolve comment {comment.get("id")}'], + [ + 'git', + 'commit', + '-m', + f'{COMMENT_RESOLUTION_COMMIT_PREFIX} {comment.get("id")}', + ], repository_path, check=False, ) @@ -850,12 +951,17 @@ def handle_failing_pipelines( run_id, ) lines = log.strip().split('\n') - context = '\n'.join(lines[-100:]) + context = '\n'.join(lines[-DEFAULT_LOG_TAIL_LINES:]) issue = f'Resolve the following failing pipeline run {run_id}:\n\n{context}' code_solver.solve_issue_round(repository_path, issue) run_cmd(['git', 'add', '.'], repository_path, check=False) run_cmd( - ['git', 'commit', '-m', f'Resolve pipeline {run_id}'], + [ + 'git', + 'commit', + '-m', + f'{PIPELINE_RESOLUTION_COMMIT_PREFIX} {run_id}', + ], repository_path, check=False, ) diff --git a/aider_gitea/__main__.py b/aider_gitea/__main__.py index e9ab35e..2152472 100644 --- a/aider_gitea/__main__.py +++ b/aider_gitea/__main__.py @@ -8,7 +8,13 @@ import argparse import logging import time -from . import RepositoryConfig, secrets, solve_issues_in_repository +from . import ( + DEFAULT_BASE_BRANCH, + DEFAULT_DAEMON_INTERVAL_SECONDS, + RepositoryConfig, + secrets, + solve_issues_in_repository, +) from .gitea_client import GiteaClient from .seen_issues_db import SeenIssuesDB @@ -31,8 +37,8 @@ def parse_args(): ) parser.add_argument( '--base-branch', - default='main', - help='Base branch to use for new branches (default: main)', + default=DEFAULT_BASE_BRANCH, + help=f'Base branch to use for new branches (default: {DEFAULT_BASE_BRANCH})', ) parser.add_argument( '--daemon', @@ -42,8 +48,8 @@ def parse_args(): parser.add_argument( '--interval', type=int, - default=30, - help='Interval in seconds between checks in daemon mode (default: 300)', + default=DEFAULT_DAEMON_INTERVAL_SECONDS, + help=f'Interval in seconds between checks in daemon mode (default: {DEFAULT_DAEMON_INTERVAL_SECONDS})', ) parser.add_argument( '--aider-model', diff --git a/test/test_claude_code_integration.py b/test/test_claude_code_integration.py index 9229225..a46d110 100644 --- a/test/test_claude_code_integration.py +++ b/test/test_claude_code_integration.py @@ -35,6 +35,12 @@ class TestClaudeCodeIntegration: assert not is_anthropic_model('') assert not is_anthropic_model(None) + # Test error handling for invalid types + with pytest.raises(TypeError): + is_anthropic_model(123) + with pytest.raises(TypeError): + is_anthropic_model(['claude']) + def test_create_code_solver_routing(self, monkeypatch): """Test that the correct solver is created based on model.""" import aider_gitea @@ -69,9 +75,10 @@ class TestClaudeCodeIntegration: 'claude', '-p', '--output-format', - 'json', - '--max-turns', - '10', + 'stream-json', + '--debug', + '--verbose', + '--dangerously-skip-permissions', issue, ] assert cmd == expected @@ -84,9 +91,10 @@ class TestClaudeCodeIntegration: 'claude', '-p', '--output-format', - 'json', - '--max-turns', - '10', + 'stream-json', + '--debug', + '--verbose', + '--dangerously-skip-permissions', '--model', 'claude-3-sonnet', issue, diff --git a/test/test_validation.py b/test/test_validation.py new file mode 100644 index 0000000..02ad648 --- /dev/null +++ b/test/test_validation.py @@ -0,0 +1,139 @@ +import pytest + +from aider_gitea import RepositoryConfig, generate_branch_name + + +class TestRepositoryConfigValidation: + """Test validation in RepositoryConfig dataclass.""" + + def test_valid_repository_config(self): + """Test that valid configurations work correctly.""" + config = RepositoryConfig( + gitea_url='https://gitea.example.com', + owner='testowner', + repo='testrepo', + base_branch='main', + ) + assert config.gitea_url == 'https://gitea.example.com' + assert config.owner == 'testowner' + assert config.repo == 'testrepo' + assert config.base_branch == 'main' + + def test_empty_gitea_url_validation(self): + """Test that empty gitea_url raises ValueError.""" + with pytest.raises(ValueError, match='gitea_url cannot be empty'): + RepositoryConfig( + gitea_url='', + owner='testowner', + repo='testrepo', + base_branch='main', + ) + + def test_empty_owner_validation(self): + """Test that empty owner raises ValueError.""" + with pytest.raises(ValueError, match='owner cannot be empty'): + RepositoryConfig( + gitea_url='https://gitea.example.com', + owner='', + repo='testrepo', + base_branch='main', + ) + + def test_empty_repo_validation(self): + """Test that empty repo raises ValueError.""" + with pytest.raises(ValueError, match='repo cannot be empty'): + RepositoryConfig( + gitea_url='https://gitea.example.com', + owner='testowner', + repo='', + base_branch='main', + ) + + def test_empty_base_branch_validation(self): + """Test that empty base_branch raises ValueError.""" + with pytest.raises(ValueError, match='base_branch cannot be empty'): + RepositoryConfig( + gitea_url='https://gitea.example.com', + owner='testowner', + repo='testrepo', + base_branch='', + ) + + def test_gitea_url_api_v1_suffix_validation(self): + """Test that gitea_url with /api/v1 suffix raises ValueError.""" + with pytest.raises( + ValueError, + match="gitea_url should not include '/api/v1' suffix", + ): + RepositoryConfig( + gitea_url='https://gitea.example.com/api/v1', + owner='testowner', + repo='testrepo', + base_branch='main', + ) + + def test_whitespace_only_fields_validation(self): + """Test that whitespace-only fields raise ValueError.""" + with pytest.raises(ValueError, match='owner cannot be empty'): + RepositoryConfig( + gitea_url='https://gitea.example.com', + owner=' ', + repo='testrepo', + base_branch='main', + ) + + +class TestBranchNameGeneration: + """Test validation in generate_branch_name function.""" + + def test_valid_branch_name_generation(self): + """Test that valid inputs generate correct branch names.""" + result = generate_branch_name('123', 'Fix authentication bug') + expected = 'issue-123-fix-authentication-bug' + assert result == expected + + def test_empty_issue_number_validation(self): + """Test that empty issue_number raises ValueError.""" + with pytest.raises(ValueError, match='Issue number cannot be empty'): + generate_branch_name('', 'Fix bug') + + def test_whitespace_issue_number_validation(self): + """Test that whitespace-only issue_number raises ValueError.""" + with pytest.raises(ValueError, match='Issue number cannot be empty'): + generate_branch_name(' ', 'Fix bug') + + def test_non_string_issue_number_validation(self): + """Test that non-string issue_number raises TypeError.""" + with pytest.raises( + TypeError, + match='Both issue_number and issue_title must be strings', + ): + generate_branch_name(123, 'Fix bug') + + def test_non_string_issue_title_validation(self): + """Test that non-string issue_title raises TypeError.""" + with pytest.raises( + TypeError, + match='Both issue_number and issue_title must be strings', + ): + generate_branch_name('123', 456) + + def test_empty_issue_title_handling(self): + """Test that empty issue_title is handled gracefully.""" + result = generate_branch_name('123', '') + expected = 'issue-123-untitled' + assert result == expected + + def test_none_issue_title_handling(self): + """Test that None issue_title is handled gracefully.""" + with pytest.raises( + TypeError, + match='Both issue_number and issue_title must be strings', + ): + generate_branch_name('123', None) + + def test_special_characters_sanitization(self): + """Test that special characters are properly sanitized.""" + result = generate_branch_name('456', 'Fix #bug [with] @special! chars?') + expected = 'issue-456-fix-bug-with-special-chars' + assert result == expected