diff --git a/apps/backend/agents/__init__.py b/apps/backend/agents/__init__.py index afa362e59..32d8cce86 100644 --- a/apps/backend/agents/__init__.py +++ b/apps/backend/agents/__init__.py @@ -7,76 +7,64 @@ This module provides: - run_autonomous_agent: Main coder agent loop - run_followup_planner: Follow-up planner for completed specs +- run_code_review_session: Code review agent for security/performance analysis - Memory management (Graphiti + file-based fallback) - Session management and post-processing - Utility functions for git and plan management -Uses lazy imports to avoid circular dependencies. +Uses lazy imports via __getattr__ to avoid circular dependencies. +Explicit re-exports below satisfy CodeQL static analysis. """ # Explicit imports required by CodeQL static analysis # (CodeQL doesn't recognize __getattr__ dynamic exports) -from .documentation_generator import run_documentation_generator_session -from .utils import sync_spec_to_source +from .code_reviewer import run_code_review_session as run_code_review_session +from .coder import run_autonomous_agent as run_autonomous_agent +from .documentation_generator import ( + run_documentation_generator_session as run_documentation_generator_session, +) +from .memory_manager import debug_memory_system_status as debug_memory_system_status +from .memory_manager import get_graphiti_context as get_graphiti_context +from .memory_manager import save_session_memory as save_session_memory +from .memory_manager import save_session_to_graphiti as save_session_to_graphiti +from .planner import run_followup_planner as run_followup_planner +from .session import post_session_processing as post_session_processing +from .session import run_agent_session as run_agent_session +from .utils import find_phase_for_subtask as find_phase_for_subtask +from .utils import find_subtask_in_plan as find_subtask_in_plan +from .utils import get_commit_count as get_commit_count +from .utils import get_latest_commit as get_latest_commit +from .utils import load_implementation_plan as load_implementation_plan +from .utils import sync_spec_to_source as sync_spec_to_source __all__ = [ + # Main API + "run_autonomous_agent", + "run_followup_planner", + "run_code_review_session", "run_documentation_generator_session", + # Memory + "debug_memory_system_status", + "get_graphiti_context", + "save_session_memory", + "save_session_to_graphiti", + # Session + "run_agent_session", + "post_session_processing", + # Utils + "get_latest_commit", + "get_commit_count", + "load_implementation_plan", + "find_subtask_in_plan", + "find_phase_for_subtask", "sync_spec_to_source", ] def __getattr__(name): - """Lazy imports to avoid circular dependencies.""" + """Lazy imports for names that may cause circular dependencies.""" if name in ("AUTO_CONTINUE_DELAY_SECONDS", "HUMAN_INTERVENTION_FILE"): from .base import AUTO_CONTINUE_DELAY_SECONDS, HUMAN_INTERVENTION_FILE - return locals()[name] - elif name == "run_autonomous_agent": - from .coder import run_autonomous_agent - - return run_autonomous_agent - elif name in ( - "debug_memory_system_status", - "get_graphiti_context", - "save_session_memory", - "save_session_to_graphiti", - ): - from .memory_manager import ( - debug_memory_system_status, - get_graphiti_context, - save_session_memory, - save_session_to_graphiti, - ) - - return locals()[name] - elif name == "run_followup_planner": - from .planner import run_followup_planner - - return run_followup_planner - elif name == "run_documentation_generator_session": - from .documentation_generator import run_documentation_generator_session - - return run_documentation_generator_session - elif name in ("post_session_processing", "run_agent_session"): - from .session import post_session_processing, run_agent_session - - return locals()[name] - elif name in ( - "find_phase_for_subtask", - "find_subtask_in_plan", - "get_commit_count", - "get_latest_commit", - "load_implementation_plan", - "sync_spec_to_source", - ): - from .utils import ( - find_phase_for_subtask, - find_subtask_in_plan, - get_commit_count, - get_latest_commit, - load_implementation_plan, - sync_spec_to_source, - ) - return locals()[name] raise AttributeError(f"module 'agents' has no attribute '{name}'") diff --git a/apps/backend/agents/code_reviewer.py b/apps/backend/agents/code_reviewer.py new file mode 100644 index 000000000..fef0c340b --- /dev/null +++ b/apps/backend/agents/code_reviewer.py @@ -0,0 +1,220 @@ +""" +Code Review Agent Session +========================== + +Runs code review sessions to analyze code changes for security issues, +performance anti-patterns, and style violations. + +Memory Integration: +- Retrieves past security patterns, gotchas, and review insights before session +- Saves code review findings (vulnerabilities, patterns, recommendations) after session +""" + +import logging +from pathlib import Path + +# Memory integration for cross-session learning +from agents.memory_manager import get_graphiti_context +from claude_agent_sdk import ClaudeSDKClient +from debug import debug, debug_detailed, debug_error, debug_section, debug_success +from task_logger import ( + LogPhase, + get_task_logger, +) + +from .session import run_agent_session + +logger = logging.getLogger(__name__) + +# Maximum characters for memory context to avoid unbounded prompt growth +_MAX_MEMORY_CONTEXT_LEN = 4000 + +# ============================================================================= +# CODE REVIEW SESSION +# ============================================================================= + + +async def run_code_review_session( + client: ClaudeSDKClient, + project_dir: Path, + spec_dir: Path, + target_files: list[str] | None = None, + pr_number: int | None = None, + review_session: int = 1, + verbose: bool = False, + previous_error: dict | None = None, +) -> tuple[str, str]: + """ + Run a code review agent session. + + Args: + client: Claude SDK client + project_dir: Project root directory (for capability detection) + spec_dir: Spec directory + target_files: Optional list of specific files to review + pr_number: Optional PR number for GitHub integration + review_session: Review iteration number + verbose: Whether to show detailed output + previous_error: Error context from previous iteration for self-correction + + Returns: + (status, response_text) where status is: + - "approved" if review approves + - "issues_found" if review finds problems + - "error" if an error occurred + """ + debug_section("code_reviewer", f"Code Review Session {review_session}") + debug( + "code_reviewer", + "Starting code review session", + spec_dir=str(spec_dir), + review_session=review_session, + target_files=target_files, + pr_number=pr_number, + ) + + logger.info("Code review session %d started", review_session) + + # Get task logger for streaming markers + task_logger = get_task_logger(spec_dir) + + # Load code review prompt with dynamically-injected project-specific MCP tools + # For now, we'll use a placeholder until subtask-1-2 creates the prompt + try: + from prompts_pkg import get_code_review_prompt + + prompt = get_code_review_prompt(spec_dir, project_dir) + debug_detailed( + "code_reviewer", + "Loaded code review prompt with project-specific tools", + prompt_length=len(prompt), + project_dir=str(project_dir), + ) + except ImportError: + # Fallback prompt for initial development + debug( + "code_reviewer", + "Code review prompt not yet available, using placeholder", + ) + prompt = f""" +You are a code review specialist agent. Your role is to analyze code changes and provide: + +1. **Security Analysis**: Identify vulnerabilities, injection risks, and unsafe patterns +2. **Performance Review**: Detect anti-patterns, inefficient algorithms, memory leaks +3. **Style & Best Practices**: Check code conventions, readability, maintainability +4. **Actionable Feedback**: Provide clear, specific recommendations for improvement + +Review the code changes in: {project_dir} +Spec directory: {spec_dir} +""" + + # Retrieve memory context for code review (past patterns, gotchas, security insights) + review_memory_context = await get_graphiti_context( + spec_dir, + project_dir, + { + "description": "Code review for security, performance, and style", + "id": f"code_reviewer_{review_session}", + }, + ) + if review_memory_context: + # Truncate memory context to prevent unbounded prompt growth + if len(review_memory_context) > _MAX_MEMORY_CONTEXT_LEN: + review_memory_context = ( + review_memory_context[:_MAX_MEMORY_CONTEXT_LEN] + "\n...(truncated)" + ) + prompt += "\n\n" + review_memory_context + logger.info("Memory context loaded for code reviewer") + debug_success("code_reviewer", "Graphiti memory context loaded for review") + + # Add session context + prompt += f"\n\n---\n\n**Review Session**: {review_session}\n" + if target_files: + prompt += f"**Target Files**: {', '.join(target_files)}\n" + if pr_number: + prompt += f"**PR Number**: {pr_number}\n" + + # Add error context for self-correction if previous iteration failed + if previous_error: + debug( + "code_reviewer", + "Adding error context for self-correction", + error_type=previous_error.get("error_type"), + consecutive_errors=previous_error.get("consecutive_errors"), + ) + prompt += f""" + +--- + +## ⚠️ CRITICAL: PREVIOUS ITERATION FAILED - SELF-CORRECTION REQUIRED + +The previous code review session failed with the following error: + +**Error**: {previous_error.get("error_message", "Unknown error")} +**Consecutive Failures**: {previous_error.get("consecutive_errors", 1)} + +### Required Action + +After completing your code review, you MUST create a review report file at: +`{spec_dir}/code_review_report.md` + +This is attempt {previous_error.get("consecutive_errors", 1) + 1}. If you fail to create the report again, the review process will be escalated to human review. + +--- + +""" + logger.warning( + "Retry with self-correction context (attempt %d)", + previous_error.get("consecutive_errors", 1) + 1, + ) + + try: + debug("code_reviewer", "Starting code review agent session...") + + # Run the agent session + status, response_text = await run_agent_session( + client=client, + message=prompt, + spec_dir=spec_dir, + verbose=verbose, + phase=LogPhase.CODING, # Use CODING phase for now + ) + + debug_success( + "code_reviewer", + f"Code review session completed with status: {status}", + ) + + # Analyze response to determine if issues were found + if status == "error": + debug_error("code_reviewer", "Code review session failed") + return ("error", response_text) + + # Check if review report was created + review_report = spec_dir / "code_review_report.md" + if review_report.exists(): + # Parse report to determine approval status + content = review_report.read_text() + if "APPROVED" in content.upper() or "NO ISSUES" in content.upper(): + debug_success("code_reviewer", "Code review approved") + return ("approved", response_text) + else: + debug("code_reviewer", "Code review found issues") + return ("issues_found", response_text) + else: + # No report created - assume issues found if session completed + debug( + "code_reviewer", + "No review report found, assuming issues for follow-up", + ) + return ("issues_found", response_text) + + except Exception as e: + debug_error("code_reviewer", f"Code review session error: {e}") + logger.exception("Code review session failed") + if task_logger: + task_logger.log_error( + f"Code review session error: {type(e).__name__}", + LogPhase.CODING, + ) + return ("error", f"Code review failed: {type(e).__name__}") diff --git a/apps/backend/prompts/code_review_agent.md b/apps/backend/prompts/code_review_agent.md new file mode 100644 index 000000000..e0b29222d --- /dev/null +++ b/apps/backend/prompts/code_review_agent.md @@ -0,0 +1,300 @@ +# Code Review Agent - Thorough Code Analysis + +You are an expert code reviewer performing deep analysis of code changes. Your goal is to review code with the rigor of a senior developer who **takes ownership of code quality**. Every line of code matters. + +## Core Principle: THOROUGH ANALYSIS REQUIRED + +**IMPORTANT**: Never skip analysis because code looks "simple" or "obvious". Even a 1-line change can: +- Break business logic +- Introduce security vulnerabilities +- Use incorrect paths or references +- Have subtle off-by-one errors +- Violate architectural patterns +- Introduce performance regressions + +**Your role is to be the last line of defense before code reaches production.** + +## Your Review Scope + +You will analyze code changes across these critical dimensions: + +### 1. Security Analysis (OWASP Top 10) +- **Injection Vulnerabilities**: SQL injection, XSS, command injection +- **Broken Authentication**: Session management, token handling, password storage +- **Sensitive Data Exposure**: Hardcoded secrets, API keys, credentials +- **Broken Access Control**: Authorization checks, permission validation +- **Security Misconfiguration**: Exposed debug endpoints, verbose error messages +- **Insecure Deserialization**: Unsafe pickle, eval, JSON parsing +- **Using Components with Known Vulnerabilities**: Outdated dependencies +- **Insufficient Logging**: Missing audit trails for security events + +### 2. Performance Analysis +- **Anti-patterns**: N+1 queries, unbounded loops, redundant computations +- **Resource Management**: Memory leaks, unclosed connections, large allocations +- **Algorithmic Complexity**: O(n²) where O(n log n) is feasible +- **Database Issues**: Missing indexes, inefficient queries, transaction overhead +- **Caching Opportunities**: Repeated expensive operations + +### 3. Code Quality & Maintainability +- **Error Handling**: Missing try/catch, swallowed exceptions, unclear error messages +- **Code Duplication**: Repeated logic that should be extracted +- **Complexity**: Functions >50 lines, deep nesting (>3 levels) +- **Naming**: Unclear variable/function names, inconsistent conventions +- **Testing**: Missing tests for new code, inadequate edge case coverage +- **Documentation**: Missing docstrings, unclear comments, outdated docs + +### 4. Best Practices & Conventions +- **Project Patterns**: Does code follow existing architectural patterns? +- **Language Idioms**: Uses language-specific best practices? +- **Framework Conventions**: Follows framework guidelines (React hooks, Django ORM, etc.)? +- **Type Safety**: Proper type annotations, avoiding `any`/`unknown` +- **Immutability**: Avoiding unintended mutations +- **SOLID Principles**: Single responsibility, dependency inversion + +### 5. Correctness & Logic +- **Off-by-one Errors**: Loop boundaries, array indexing +- **Null/Undefined Handling**: Missing null checks, potential crashes +- **Edge Cases**: Empty arrays, zero/negative values, boundary conditions +- **Race Conditions**: Async operations, shared state +- **Business Logic**: Does the code actually solve the stated problem? +- **Path Correctness**: Do file paths, URLs, imports actually exist and work? + +--- + +## Your Review Process + +### Phase 1: Understand the Change (ALWAYS DO THIS) + +Use extended thinking to understand: +``` +What is this change trying to accomplish? +- New feature? Bug fix? Refactor? Performance optimization? +- What problem does it solve? +- What are the acceptance criteria? +- What files are affected and why? +``` + +**Read EVERY file** in the changeset. No skipping. + +### Phase 2: Analyze for Issues (SYSTEMATIC REVIEW) + +For EVERY file changed, analyze using this checklist: + +#### Security Checklist +- [ ] No hardcoded secrets, API keys, passwords +- [ ] Input validation present for user data +- [ ] SQL/NoSQL queries use parameterization +- [ ] Authentication/authorization checks in place +- [ ] No unsafe deserialization (eval, pickle without validation) +- [ ] Error messages don't leak sensitive data +- [ ] HTTPS/TLS for sensitive communications + +#### Performance Checklist +- [ ] No N+1 query patterns +- [ ] Database queries optimized (indexes, batch operations) +- [ ] No unbounded loops or recursion +- [ ] Resources properly cleaned up (close connections, free memory) +- [ ] Caching used for expensive operations +- [ ] Async operations where appropriate + +#### Quality Checklist +- [ ] Error handling covers failure scenarios +- [ ] No swallowed exceptions (catch-and-ignore) +- [ ] Functions are single-purpose (<50 lines) +- [ ] Nesting depth reasonable (<3 levels) +- [ ] No code duplication +- [ ] Variable/function names are descriptive +- [ ] Edge cases handled (null, empty, boundaries) + +#### Best Practices Checklist +- [ ] Follows project patterns and conventions +- [ ] Type annotations present (TypeScript, Python, etc.) +- [ ] Tests added for new code +- [ ] Documentation updated +- [ ] No commented-out code +- [ ] Imports organized and necessary + +#### Correctness Checklist +- [ ] Logic is correct (no inverted conditions) +- [ ] Paths and references exist +- [ ] Boundary conditions handled +- [ ] Null/undefined checked before use +- [ ] Async operations awaited properly +- [ ] Business logic matches requirements + +### Phase 3: Generate Findings (ACTIONABLE FEEDBACK) + +For each issue found, create a finding with: + +**Severity Levels:** +- **CRITICAL**: Security vulnerability, data loss risk, production-breaking bug +- **HIGH**: Performance degradation, broken functionality, maintainability hazard +- **MEDIUM**: Code quality issue, minor security concern, testability problem +- **LOW**: Style inconsistency, documentation gap, minor optimization + +**Finding Format:** +```markdown +### [SEVERITY] Issue Title + +**File:** `path/to/file.ts` (Line X-Y) + +**Problem:** +Clear description of what's wrong + +**Impact:** +Why this matters (security risk, performance, maintainability) + +**Recommendation:** +Specific action to fix (with code example if helpful) + +**Code Example:** +```language +// ❌ Current (problematic) +current code + +// ✅ Suggested (fixed) +fixed code +``` +``` + +### Phase 4: Prioritize & Categorize + +Group findings by: +1. **Critical Issues** - Must fix before merge +2. **High Priority** - Should fix before merge +3. **Medium Priority** - Consider fixing before merge +4. **Low Priority** - Nice to have, non-blocking + +**Quality Gates:** +- **CRITICAL issues** → Must be fixed +- **HIGH issues** → Must be fixed +- **MEDIUM issues** → Should be fixed (AI can fix quickly) +- **LOW issues** → Suggest for improvement + +### Phase 5: Generate Review Report + +Provide a structured review with: + +1. **Summary**: Overview of changes and scope +2. **Verdict**: APPROVE / REQUEST_CHANGES / NEEDS_REVISION +3. **Critical Issues**: Blocking problems +4. **High Priority Issues**: Important concerns +5. **Medium Priority Issues**: Quality improvements +6. **Low Priority Suggestions**: Optional enhancements +7. **Positive Highlights**: What was done well + +--- + +## Review Strategies by Change Type + +### For Bug Fixes +- **Verify the fix actually addresses the root cause** +- Check if the same bug exists elsewhere +- Ensure tests prevent regression +- Validate edge cases are covered + +### For New Features +- **Security**: Authentication, authorization, input validation +- **Performance**: Database queries, async operations, caching +- **Testing**: Unit tests, integration tests, edge cases +- **Documentation**: API docs, usage examples, comments + +### For Refactors +- **Correctness**: Behavior unchanged (verify with tests) +- **Simplification**: Actually simpler, not more complex +- **Coverage**: Tests still pass and cover refactored code +- **Patterns**: Follows project conventions + +### For Performance Optimizations +- **Measurement**: Benchmark data to prove improvement +- **Trade-offs**: Complexity vs speed (is it worth it?) +- **Correctness**: Optimization doesn't break functionality +- **Edge Cases**: Fast path doesn't skip validation + +--- + +## Available Tools + +You have access to: + +### Code Analysis +- **Read**: Read files to analyze code +- **Grep**: Search for patterns across codebase +- **Glob**: Find files by pattern +- **Bash**: Run linters, tests, security scanners + +### Testing & Validation +- **Run Tests**: Execute test suite to verify changes +- **Run Linters**: Check style and conventions +- **Security Scanners**: Run tools like bandit (Python), semgrep, npm audit + +### Example Tool Usage: + +```typescript +// Search for similar patterns in codebase +grep("SQL.*execute", { output_mode: "files_with_matches" }) + +// Find all authentication-related files +glob("**/*auth*.{ts,py,js}") + +// Run tests to verify changes +bash("npm test") + +// Check for security vulnerabilities +bash("npm audit") +``` + +--- + +## Review Output Format + +Your review should be structured as follows: + +```markdown +# Code Review Report + +## Summary +[1-2 sentence overview of changes] + +## Verdict +**[APPROVE / REQUEST_CHANGES / NEEDS_REVISION]** + +## Changes Reviewed +- `file1.ts` - [Brief description] +- `file2.py` - [Brief description] + +## Critical Issues (Must Fix) +[List CRITICAL findings with details] + +## High Priority Issues (Should Fix) +[List HIGH findings with details] + +## Medium Priority Issues (Consider Fixing) +[List MEDIUM findings with details] + +## Low Priority Suggestions (Optional) +[List LOW findings with details] + +## Positive Highlights +[What was done well - be specific] + +## Testing Recommendations +[What tests should be added/run] + +## Overall Assessment +[Final thoughts and next steps] +``` + +--- + +## Key Principles + +1. **Be Thorough**: Review every line with care +2. **Be Specific**: Point to exact files and lines +3. **Be Actionable**: Provide clear fix recommendations +4. **Be Constructive**: Highlight what's done well +5. **Be Consistent**: Apply same standards across all code +6. **Be Security-Minded**: Assume malicious input +7. **Be Performance-Aware**: Consider scale and growth + +**Remember**: Your job is to catch issues before they reach production. Be rigorous, be helpful, be professional. diff --git a/apps/backend/prompts_pkg/__init__.py b/apps/backend/prompts_pkg/__init__.py index 71bcfe67f..c1fa61afe 100644 --- a/apps/backend/prompts_pkg/__init__.py +++ b/apps/backend/prompts_pkg/__init__.py @@ -24,6 +24,7 @@ # Import all functions from prompts from .prompts import ( + get_code_review_prompt, get_coding_prompt, get_followup_planner_prompt, get_planner_prompt, @@ -46,6 +47,7 @@ "get_followup_planner_prompt", "get_qa_reviewer_prompt", "get_qa_fixer_prompt", + "get_code_review_prompt", "is_first_run", # project_context functions "load_project_index", diff --git a/apps/backend/prompts_pkg/prompts.py b/apps/backend/prompts_pkg/prompts.py index 684f31b7c..9bbf7a890 100644 --- a/apps/backend/prompts_pkg/prompts.py +++ b/apps/backend/prompts_pkg/prompts.py @@ -652,6 +652,34 @@ def get_qa_fixer_prompt(spec_dir: Path, project_dir: Path) -> str: return spec_context + base_prompt +def get_code_review_prompt(spec_dir: Path, project_dir: Path) -> str: + """ + Load the code review agent prompt with spec paths injected. + + Args: + spec_dir: Directory containing the spec files + project_dir: Root directory of the project + + Returns: + The code review agent prompt content with paths injected + """ + base_prompt = _load_prompt_file("code_review_agent.md") + + spec_context = f"""## SPEC LOCATION + +Your spec and progress files are located at: +- Spec: `{spec_dir}/spec.md` +- Implementation plan: `{spec_dir}/implementation_plan.json` +- Code review output: `{spec_dir}/code_review_report.md` + +The project root is: `{project_dir}` + +--- + +""" + return spec_context + base_prompt + + def load_custom_template_prompt(template_name: str, project_dir: Path) -> str: """ Load a custom agent template prompt by name. diff --git a/apps/backend/runners/github/orchestrator.py b/apps/backend/runners/github/orchestrator.py index c1d4ea08b..c991dd1a9 100644 --- a/apps/backend/runners/github/orchestrator.py +++ b/apps/backend/runners/github/orchestrator.py @@ -49,10 +49,10 @@ from .services.io_utils import safe_print except (ImportError, ValueError, SystemError): # When imported directly (runner.py adds github dir to path) - from bot_detection import BotDetector - from context_gatherer import PRContext, PRContextGatherer - from gh_client import GHClient - from models import ( + from runners.github.bot_detection import BotDetector + from runners.github.context_gatherer import PRContext, PRContextGatherer + from runners.github.gh_client import GHClient + from runners.github.models import ( BRANCH_BEHIND_BLOCKER_MSG, BRANCH_BEHIND_REASONING, AICommentTriage, @@ -67,15 +67,15 @@ StructuralIssue, TriageResult, ) - from permissions import GitHubPermissionChecker - from rate_limiter import RateLimiter - from services import ( + from runners.github.permissions import GitHubPermissionChecker + from runners.github.rate_limiter import RateLimiter + from runners.github.services import ( AutoFixProcessor, BatchProcessor, PRReviewEngine, TriageEngine, ) - from services.io_utils import safe_print + from runners.github.services.io_utils import safe_print @dataclass @@ -640,8 +640,8 @@ async def followup_review_pr(self, pr_number: int) -> PRReviewResult: from .context_gatherer import FollowupContextGatherer from .services.followup_reviewer import FollowupReviewer except (ImportError, ValueError, SystemError): - from context_gatherer import FollowupContextGatherer - from services.followup_reviewer import FollowupReviewer + from runners.github.context_gatherer import FollowupContextGatherer + from runners.github.services.followup_reviewer import FollowupReviewer # Gather follow-up context gatherer = FollowupContextGatherer( @@ -875,7 +875,7 @@ def is_ci_blocker(b: str) -> bool: ParallelFollowupReviewer, ) except (ImportError, ValueError, SystemError): - from services.parallel_followup_reviewer import ( + from runners.github.services.parallel_followup_reviewer import ( ParallelFollowupReviewer, ) @@ -1378,6 +1378,123 @@ def _format_review_body(self, result: PRReviewResult) -> str: """Format the review body for posting to GitHub.""" return result.summary + # ========================================================================= + # CODE REVIEW WORKFLOW + # ========================================================================= + + async def code_review_pr(self, pr_number: int) -> list[PRReviewFinding]: + """ + Run security-focused code review on a pull request. + + Performs: + - Security vulnerability scanning (secrets, SAST) + - Converts security findings to PR review findings + - Does NOT post to GitHub (for manual review of security issues) + + Args: + pr_number: The PR number to review + + Returns: + List of PRReviewFinding objects with security issues found + + Raises: + Exception: If review fails + """ + safe_print( + f"[DEBUG orchestrator] code_review_pr() called for PR #{pr_number}", + flush=True, + ) + + self._report_progress( + "gathering_context", + 10, + f"Gathering context for PR #{pr_number}...", + pr_number=pr_number, + ) + + try: + # Gather PR context + safe_print("[DEBUG orchestrator] Creating context gatherer...") + gatherer = PRContextGatherer( + self.project_dir, pr_number, repo=self.config.repo + ) + + safe_print("[DEBUG orchestrator] Gathering PR context...") + pr_context = await gatherer.gather() + if pr_context is None: + raise RuntimeError(f"Failed to gather context for PR #{pr_number}") + safe_print( + f"[DEBUG orchestrator] Context gathered: {pr_context.title} " + f"({len(pr_context.changed_files)} files changed)", + flush=True, + ) + + self._report_progress( + "analyzing", + 30, + "Running security scan...", + pr_number=pr_number, + ) + + # Import code review service + try: + from .services.code_review_service import CodeReviewService + except (ImportError, ValueError, SystemError): + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + # Create code review service + code_review_service = CodeReviewService( + project_dir=self.project_dir, + github_dir=self.github_dir, + config=self.config, + progress_callback=self.progress_callback, + ) + + # Run code review (security scan) + safe_print("[DEBUG orchestrator] Running security-focused code review...") + findings = await code_review_service.review_code_changes( + context=pr_context, + changed_files=None, # Scan all changed files + ) + + safe_print( + f"[DEBUG orchestrator] Code review complete: {len(findings)} findings", + flush=True, + ) + + # Get summary statistics + summary = code_review_service.get_findings_summary(findings) + by_sev = summary.get("by_severity", {}) + safe_print( + f"[CodeReview] Summary: {summary.get('total', 0)} total findings " + f"({by_sev.get('critical', 0)} critical, " + f"{by_sev.get('high', 0)} high)", + flush=True, + ) + + self._report_progress( + "complete", + 100, + f"Code review complete: {len(findings)} findings", + pr_number=pr_number, + ) + + return findings + + except Exception as e: + import traceback + + error_details = f"{type(e).__name__}: {e}" + full_traceback = traceback.format_exc() + safe_print( + f"[ERROR orchestrator] Code review failed for PR #{pr_number}: {error_details}", + flush=True, + ) + safe_print(f"[ERROR orchestrator] Full traceback:\n{full_traceback}") + raise + # ========================================================================= # ISSUE TRIAGE WORKFLOW # ========================================================================= diff --git a/apps/backend/runners/github/runner.py b/apps/backend/runners/github/runner.py index c765b05de..ccf69edc9 100644 --- a/apps/backend/runners/github/runner.py +++ b/apps/backend/runners/github/runner.py @@ -328,6 +328,96 @@ async def cmd_followup_review_pr(args) -> int: return 1 +async def cmd_code_review_pr(args) -> int: + """Run security-focused code review on a pull request.""" + import sys + + # Force unbuffered output so Electron sees it in real-time + if hasattr(sys.stdout, "reconfigure"): + sys.stdout.reconfigure(line_buffering=True) + if hasattr(sys.stderr, "reconfigure"): + sys.stderr.reconfigure(line_buffering=True) + + debug = os.environ.get("DEBUG") + if debug: + safe_print(f"[DEBUG] Starting code review for PR #{args.pr_number}") + safe_print(f"[DEBUG] Project directory: {args.project}") + safe_print("[DEBUG] Building config...") + + config = get_config(args) + + if debug: + safe_print( + f"[DEBUG] Config built: repo={config.repo}, model={config.model}", + flush=True, + ) + safe_print("[DEBUG] Creating orchestrator...") + + orchestrator = GitHubOrchestrator( + project_dir=args.project, + config=config, + progress_callback=print_progress, + ) + + if debug: + safe_print("[DEBUG] Orchestrator created") + safe_print( + f"[DEBUG] Calling orchestrator.code_review_pr({args.pr_number})...", + flush=True, + ) + + try: + findings = await orchestrator.code_review_pr(args.pr_number) + except Exception as e: + safe_print(f"\nCode review failed: {e}") + return 1 + + if debug: + safe_print( + f"[DEBUG] code_review_pr returned {len(findings)} findings", flush=True + ) + + # Print results + safe_print(f"\n{'=' * 60}") + safe_print(f"PR #{args.pr_number} Code Review Complete") + safe_print(f"{'=' * 60}") + safe_print(f"Total Findings: {len(findings)}") + + if findings: + # Count by severity + critical = sum(1 for f in findings if f.severity.value == "critical") + high = sum(1 for f in findings if f.severity.value == "high") + medium = sum(1 for f in findings if f.severity.value == "medium") + low = sum(1 for f in findings if f.severity.value == "low") + + safe_print("\nBy Severity:") + if critical: + safe_print(f" CRITICAL: {critical}") + if high: + safe_print(f" HIGH: {high}") + if medium: + safe_print(f" MEDIUM: {medium}") + if low: + safe_print(f" LOW: {low}") + + safe_print("\nFindings:") + for f in findings: + emoji = {"critical": "🔴", "high": "🟠", "medium": "🟡", "low": "🟢"} + safe_print( + f"\n {emoji.get(f.severity.value, '⚪')} [{f.severity.value.upper()}] {f.title}" + ) + safe_print(f" File: {f.file}:{f.line}") + safe_print(f" Category: {f.category.value}") + if f.description: + # Print first line of description + first_line = f.description.split("\n")[0] + safe_print(f" {first_line}") + else: + safe_print("\n✅ No security issues found!") + + return 0 + + async def cmd_triage(args) -> int: """Triage issues.""" config = get_config(args) @@ -704,6 +794,13 @@ def main(): ) followup_parser.add_argument("pr_number", type=int, help="PR number to review") + # code-review-pr command + code_review_parser = subparsers.add_parser( + "code-review-pr", + help="Run security-focused code review on a PR", + ) + code_review_parser.add_argument("pr_number", type=int, help="PR number to review") + # triage command triage_parser = subparsers.add_parser("triage", help="Triage issues") triage_parser.add_argument( @@ -793,6 +890,7 @@ def main(): commands = { "review-pr": cmd_review_pr, "followup-review-pr": cmd_followup_review_pr, + "code-review-pr": cmd_code_review_pr, "triage": cmd_triage, "auto-fix": cmd_auto_fix, "check-auto-fix-labels": cmd_check_labels, diff --git a/apps/backend/runners/github/services/autofix_processor.py b/apps/backend/runners/github/services/autofix_processor.py index a76b7b400..3d01d7e2a 100644 --- a/apps/backend/runners/github/services/autofix_processor.py +++ b/apps/backend/runners/github/services/autofix_processor.py @@ -13,8 +13,8 @@ from ..models import AutoFixState, AutoFixStatus, GitHubRunnerConfig from ..permissions import GitHubPermissionChecker except (ImportError, ValueError, SystemError): - from models import AutoFixState, AutoFixStatus, GitHubRunnerConfig - from permissions import GitHubPermissionChecker + from runners.github.models import AutoFixState, AutoFixStatus, GitHubRunnerConfig + from runners.github.permissions import GitHubPermissionChecker class AutoFixProcessor: diff --git a/apps/backend/runners/github/services/batch_processor.py b/apps/backend/runners/github/services/batch_processor.py index bd1ecb53b..f53e39244 100644 --- a/apps/backend/runners/github/services/batch_processor.py +++ b/apps/backend/runners/github/services/batch_processor.py @@ -13,8 +13,8 @@ from ..models import AutoFixState, AutoFixStatus, GitHubRunnerConfig from .io_utils import safe_print except (ImportError, ValueError, SystemError): - from models import AutoFixState, AutoFixStatus, GitHubRunnerConfig - from services.io_utils import safe_print + from runners.github.models import AutoFixState, AutoFixStatus, GitHubRunnerConfig + from runners.github.services.io_utils import safe_print class BatchProcessor: diff --git a/apps/backend/runners/github/services/category_utils.py b/apps/backend/runners/github/services/category_utils.py index 9c1d7d234..04be6f76a 100644 --- a/apps/backend/runners/github/services/category_utils.py +++ b/apps/backend/runners/github/services/category_utils.py @@ -13,7 +13,7 @@ try: from ..models import ReviewCategory except (ImportError, ValueError, SystemError): - from models import ReviewCategory + from runners.github.models import ReviewCategory # Map AI-generated category names to valid ReviewCategory enum values diff --git a/apps/backend/runners/github/services/code_review_service.py b/apps/backend/runners/github/services/code_review_service.py new file mode 100644 index 000000000..869309d31 --- /dev/null +++ b/apps/backend/runners/github/services/code_review_service.py @@ -0,0 +1,550 @@ +""" +Code Review Service +=================== + +Integrates security scanner with PR code review workflow. +Converts security vulnerabilities to PR review findings and provides +a unified interface for code review operations. + +Usage: + from runners.github.services.code_review_service import CodeReviewService + + service = CodeReviewService(project_dir, github_dir, config) + findings = await service.review_code_changes(context) +""" + +from __future__ import annotations + +import hashlib +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +try: + from ...analysis.security_scanner import SecurityScanner, SecurityVulnerability + from ..context_gatherer import PRContext + from ..gh_client import GHClient + from ..models import ( + GitHubRunnerConfig, + PRReviewFinding, + ReviewCategory, + ReviewSeverity, + ) + from .io_utils import safe_print +except (ImportError, ValueError, SystemError): + from analysis.security_scanner import SecurityScanner, SecurityVulnerability + from runners.github.context_gatherer import PRContext + from runners.github.gh_client import GHClient + from runners.github.models import ( + GitHubRunnerConfig, + PRReviewFinding, + ReviewCategory, + ReviewSeverity, + ) + from runners.github.services.io_utils import safe_print + + +# Define a local ProgressCallback to avoid circular import +@dataclass +class ProgressCallback: + """Callback for progress updates - local definition to avoid circular import.""" + + phase: str + progress: int + message: str + pr_number: int | None = None + extra: dict[str, Any] | None = None + + +class CodeReviewService: + """ + Service for running code reviews using security scanner. + + Integrates with the security scanner to detect vulnerabilities + and converts them to PR review findings. + """ + + def __init__( + self, + project_dir: Path, + github_dir: Path, + config: GitHubRunnerConfig, + progress_callback=None, + ): + """ + Initialize code review service. + + Args: + project_dir: Project root directory + github_dir: GitHub automation directory + config: GitHub runner configuration + progress_callback: Optional callback for progress updates + """ + self.project_dir = Path(project_dir) + self.github_dir = Path(github_dir) + self.config = config + self.progress_callback = progress_callback + self.scanner = SecurityScanner() + + def _report_progress(self, phase: str, progress: int, message: str, **kwargs): + """Report progress if callback is set.""" + if self.progress_callback: + self.progress_callback( + ProgressCallback( + phase=phase, progress=progress, message=message, **kwargs + ) + ) + + def _generate_finding_id(self, file: str, line: int, title: str) -> str: + """Generate unique finding ID from file, line, and title.""" + key = f"{file}:{line}:{title}" + return hashlib.sha256(key.encode()).hexdigest()[:12] + + def _map_severity(self, scanner_severity: str) -> ReviewSeverity: + """ + Map security scanner severity to PR review severity. + + Args: + scanner_severity: Severity from security scanner (critical, high, medium, low, info) + + Returns: + ReviewSeverity enum value + """ + severity_map = { + "critical": ReviewSeverity.CRITICAL, + "high": ReviewSeverity.HIGH, + "medium": ReviewSeverity.MEDIUM, + "low": ReviewSeverity.LOW, + "info": ReviewSeverity.LOW, + } + key = str(scanner_severity).lower() if scanner_severity else "" + return severity_map.get(key, ReviewSeverity.MEDIUM) + + def _map_category(self, scanner_source: str) -> ReviewCategory: + """ + Map security scanner source to PR review category. + + Args: + scanner_source: Source scanner (secrets, bandit, npm_audit, etc.) + + Returns: + ReviewCategory enum value + """ + category_map = { + "secrets": ReviewCategory.SECURITY, + "bandit": ReviewCategory.SECURITY, + "npm_audit": ReviewCategory.SECURITY, + "semgrep": ReviewCategory.SECURITY, + "dependency_check": ReviewCategory.SECURITY, + } + key = str(scanner_source).lower() if scanner_source else "" + return category_map.get(key, ReviewCategory.SECURITY) + + def _convert_vulnerability_to_finding( + self, vuln: SecurityVulnerability + ) -> PRReviewFinding: + """ + Convert security vulnerability to PR review finding. + + Args: + vuln: Security vulnerability from scanner + + Returns: + PRReviewFinding object + """ + file_path = vuln.file if vuln.file else "project-wide" + line_num = vuln.line if vuln.line else 1 + + finding_id = self._generate_finding_id(file_path, line_num, vuln.title) + + # Build description with CWE reference if available + description = vuln.description or "" + if vuln.cwe: + description += f"\n\n**CWE Reference**: {vuln.cwe}" + + return PRReviewFinding( + id=finding_id, + severity=self._map_severity(vuln.severity), + category=self._map_category(vuln.source), + title=vuln.title, + description=description, + file=file_path, + line=line_num, + end_line=None, + suggested_fix=None, # Security scanner doesn't provide fixes + fixable=False, # Manual review required for security issues + evidence=f"Detected by {vuln.source} scanner", + confidence=0.8, # Security scanners are generally reliable + source_agents=["security_scanner"], + ) + + def _convert_secret_to_finding(self, secret: dict) -> PRReviewFinding: + """ + Convert detected secret to PR review finding. + + Args: + secret: Secret detection result + + Returns: + PRReviewFinding object + """ + file_path = secret.get("file", "unknown") + line_num = secret.get("line", 1) + pattern = secret.get("pattern", "unknown") + + finding_id = self._generate_finding_id( + file_path, line_num, f"Secret detected: {pattern}" + ) + + return PRReviewFinding( + id=finding_id, + severity=ReviewSeverity.CRITICAL, # Secrets are always critical + category=ReviewCategory.SECURITY, + title=f"Detected secret: {pattern}", + description=( + f"A potential secret or credential was detected in this file.\n\n" + f"**Pattern**: {pattern}\n" + f"**Matched Text**: [REDACTED]\n\n" + "**Action Required**: Remove this secret immediately and rotate the credential." + ), + file=file_path, + line=line_num, + end_line=None, + suggested_fix="Remove the secret and use environment variables or a secrets manager", + fixable=False, # Requires manual intervention + evidence=f"Matched pattern: {pattern}", + confidence=0.9, # High confidence for pattern-based detection + source_agents=["secrets_scanner"], + ) + + async def review_code_changes( + self, + context: PRContext, + changed_files: list[str] | None = None, + ) -> list[PRReviewFinding]: + """ + Run code review on changed files using security scanner. + + Args: + context: PR context with changed files and metadata + changed_files: Optional list of specific files to scan (if None, uses all changed files) + + Returns: + List of PR review findings + """ + self._report_progress( + "code_review", + 10, + "Starting security scan...", + pr_number=getattr(context, "pr_number", None), + ) + + safe_print("[CodeReview] Running security scanner...", flush=True) + + # Extract file paths from context + if changed_files is None: + # Handle both object.path and plain strings + changed_files = [getattr(f, "path", f) for f in context.changed_files] + + # Run security scan (offload blocking I/O to a thread) + import asyncio + + spec_dir = self.github_dir / "pr" / f"pr_{getattr(context, 'pr_number', 0)}" + scan_result = await asyncio.to_thread( + self.scanner.scan, + project_dir=self.project_dir, + spec_dir=spec_dir if spec_dir.exists() else None, + changed_files=changed_files, + run_secrets=True, + run_sast=True, + run_dependency_audit=False, # Skip dependency audit for code review + ) + + self._report_progress( + "code_review", + 50, + f"Security scan complete: {len(scan_result.vulnerabilities)} issues, {len(scan_result.secrets)} secrets", + pr_number=getattr(context, "pr_number", None), + ) + + safe_print( + f"[CodeReview] Found {len(scan_result.vulnerabilities)} vulnerabilities, {len(scan_result.secrets)} secrets", + flush=True, + ) + + # Convert scan results to PR findings + findings: list[PRReviewFinding] = [] + + # Convert secrets + for secret in scan_result.secrets: + finding = self._convert_secret_to_finding(secret) + findings.append(finding) + safe_print( + f"[CodeReview] Secret detected: {finding.file}:{finding.line}", + flush=True, + ) + + # Convert vulnerabilities + for vuln in scan_result.vulnerabilities: + finding = self._convert_vulnerability_to_finding(vuln) + findings.append(finding) + safe_print( + f"[CodeReview] {vuln.severity.upper()} issue: {vuln.title} in {vuln.file or 'project-wide'}", + flush=True, + ) + + # Report scan errors if any + if scan_result.scan_errors: + safe_print( + f"[CodeReview] Warning: {len(scan_result.scan_errors)} scan errors occurred", + flush=True, + ) + for error in scan_result.scan_errors: + safe_print(f" - {error}", flush=True) + + self._report_progress( + "code_review", + 100, + f"Code review complete: {len(findings)} findings", + pr_number=getattr(context, "pr_number", None), + extra={ + "findings_count": len(findings), + "critical_count": sum( + 1 for f in findings if f.severity == ReviewSeverity.CRITICAL + ), + "high_count": sum( + 1 for f in findings if f.severity == ReviewSeverity.HIGH + ), + }, + ) + + safe_print( + f"[CodeReview] Review complete: {len(findings)} total findings", flush=True + ) + + return findings + + def should_block_merge(self, findings: list[PRReviewFinding]) -> bool: + """ + Determine if findings should block merge. + + Args: + findings: List of PR review findings + + Returns: + True if merge should be blocked (critical issues found) + """ + critical_count = sum( + 1 for f in findings if f.severity == ReviewSeverity.CRITICAL + ) + return critical_count > 0 + + def get_findings_summary(self, findings: list[PRReviewFinding]) -> dict: + """ + Get summary statistics for findings. + + Args: + findings: List of PR review findings + + Returns: + Dictionary with summary statistics + """ + severity_counts = { + "critical": sum( + 1 for f in findings if f.severity == ReviewSeverity.CRITICAL + ), + "high": sum(1 for f in findings if f.severity == ReviewSeverity.HIGH), + "medium": sum(1 for f in findings if f.severity == ReviewSeverity.MEDIUM), + "low": sum(1 for f in findings if f.severity == ReviewSeverity.LOW), + } + + category_counts = {} + for finding in findings: + # Handle both enum.value and plain string + cat = getattr(finding.category, "value", finding.category) + category_counts[cat] = category_counts.get(cat, 0) + 1 + + return { + "total": len(findings), + "by_severity": severity_counts, + "by_category": category_counts, + "should_block": self.should_block_merge(findings), + } + + def _format_review_body(self, findings: list[PRReviewFinding]) -> str: + """ + Format findings into a markdown review body. + + Args: + findings: List of PR review findings + + Returns: + Markdown formatted review body + """ + if not findings: + return "✅ No security issues found in this PR." + + summary = self.get_findings_summary(findings) + + # Build header with summary + body_parts = ["## 🔒 Security Review Results\n"] + + # Add severity summary + severity_counts = summary["by_severity"] + if severity_counts["critical"] > 0: + body_parts.append(f"🚨 **Critical**: {severity_counts['critical']}") + if severity_counts["high"] > 0: + body_parts.append(f"⚠️ **High**: {severity_counts['high']}") + if severity_counts["medium"] > 0: + body_parts.append(f"⚡ **Medium**: {severity_counts['medium']}") + if severity_counts["low"] > 0: + body_parts.append(f"ℹ️ **Low**: {severity_counts['low']}") + + body_parts.append(f"\n**Total Issues**: {summary['total']}\n") + + # Add merge recommendation + if summary["should_block"]: + body_parts.append( + "❌ **Recommendation**: Do not merge until critical issues are resolved.\n" + ) + else: + body_parts.append("⚠️ **Recommendation**: Review findings before merging.\n") + + # Group findings by severity + by_severity = { + ReviewSeverity.CRITICAL: [], + ReviewSeverity.HIGH: [], + ReviewSeverity.MEDIUM: [], + ReviewSeverity.LOW: [], + } + + for finding in findings: + by_severity[finding.severity].append(finding) + + # Add findings in severity order + for severity in [ + ReviewSeverity.CRITICAL, + ReviewSeverity.HIGH, + ReviewSeverity.MEDIUM, + ReviewSeverity.LOW, + ]: + severity_findings = by_severity[severity] + if not severity_findings: + continue + + severity_icons = { + ReviewSeverity.CRITICAL: "🚨", + ReviewSeverity.HIGH: "⚠️", + ReviewSeverity.MEDIUM: "⚡", + ReviewSeverity.LOW: "ℹ️", + } + + body_parts.append( + f"\n### {severity_icons[severity]} {severity.value.title()} Severity\n" + ) + + for finding in severity_findings: + body_parts.append(f"#### {finding.title}\n") + body_parts.append(f"**Location**: `{finding.file}:{finding.line}`\n") + body_parts.append(f"{finding.description}\n") + + if finding.suggested_fix: + body_parts.append(f"**Suggested Fix**: {finding.suggested_fix}\n") + + if finding.evidence: + body_parts.append(f"**Evidence**: {finding.evidence}\n") + + body_parts.append("") # Empty line between findings + + # Add footer + body_parts.append("\n---") + body_parts.append("*Automated security review by Auto-Claude*") + + return "\n".join(body_parts) + + async def post_review_to_github( + self, + pr_number: int, + findings: list[PRReviewFinding], + repo: str | None = None, + ) -> int: + """ + Post review findings to GitHub PR via gh_client. + + Args: + pr_number: PR number to post review to + findings: List of PR review findings + repo: Optional repository in 'owner/repo' format + + Returns: + Review ID (currently 0, as gh CLI doesn't return ID) + + Raises: + GHCommandError: If posting review fails + """ + self._report_progress( + "post_review", + 10, + f"Posting review to PR #{pr_number}...", + pr_number=pr_number, + ) + + safe_print(f"[CodeReview] Posting review to PR #{pr_number}...", flush=True) + + # Initialize GH client + gh_client = GHClient( + project_dir=self.project_dir, + repo=repo, + ) + + # Format review body + review_body = self._format_review_body(findings) + + # Determine review event based on findings + event = "comment" + if self.should_block_merge(findings): + event = "request-changes" + safe_print( + f"[CodeReview] Requesting changes due to {sum(1 for f in findings if f.severity == ReviewSeverity.CRITICAL)} critical issues", + flush=True, + ) + elif not findings: + event = "approve" + safe_print("[CodeReview] Approving PR - no issues found", flush=True) + else: + safe_print( + "[CodeReview] Posting comment review with non-critical findings", + flush=True, + ) + + # Post review using gh_client + try: + review_id = await gh_client.pr_review( + pr_number=pr_number, + body=review_body, + event=event, + ) + except Exception as exc: + safe_print( + f"[CodeReview] Failed to post review to PR #{pr_number}: {type(exc).__name__}", + flush=True, + ) + raise + + self._report_progress( + "post_review", + 100, + f"Review posted to PR #{pr_number}", + pr_number=pr_number, + extra={ + "review_id": review_id, + "event": event, + "findings_count": len(findings), + }, + ) + + safe_print( + f"[CodeReview] Review posted successfully (event: {event})", flush=True + ) + + return review_id diff --git a/apps/backend/runners/github/services/followup_reviewer.py b/apps/backend/runners/github/services/followup_reviewer.py index 03c3097a0..2b0fbfee7 100644 --- a/apps/backend/runners/github/services/followup_reviewer.py +++ b/apps/backend/runners/github/services/followup_reviewer.py @@ -39,18 +39,18 @@ from .prompt_manager import PromptManager from .pydantic_models import FollowupReviewResponse except (ImportError, ValueError, SystemError): - from gh_client import GHClient - from models import ( + from runners.github.gh_client import GHClient + from runners.github.models import ( MergeVerdict, PRReviewFinding, PRReviewResult, ReviewCategory, ReviewSeverity, ) - from services.category_utils import map_category - from services.io_utils import safe_print - from services.prompt_manager import PromptManager - from services.pydantic_models import FollowupReviewResponse + from runners.github.services.category_utils import map_category + from runners.github.services.io_utils import safe_print + from runners.github.services.prompt_manager import PromptManager + from runners.github.services.pydantic_models import FollowupReviewResponse logger = logging.getLogger(__name__) diff --git a/apps/backend/runners/github/services/parallel_followup_reviewer.py b/apps/backend/runners/github/services/parallel_followup_reviewer.py index a7e97bfe3..6325c9950 100644 --- a/apps/backend/runners/github/services/parallel_followup_reviewer.py +++ b/apps/backend/runners/github/services/parallel_followup_reviewer.py @@ -49,10 +49,11 @@ from .pydantic_models import ParallelFollowupResponse from .sdk_utils import process_sdk_stream except (ImportError, ValueError, SystemError): - from context_gatherer import _validate_git_ref from core.client import create_client - from gh_client import GHClient - from models import ( + from phase_config import get_thinking_budget, resolve_model_id + from runners.github.context_gatherer import _validate_git_ref + from runners.github.gh_client import GHClient + from runners.github.models import ( BRANCH_BEHIND_BLOCKER_MSG, BRANCH_BEHIND_REASONING, GitHubRunnerConfig, @@ -61,12 +62,11 @@ PRReviewResult, ReviewSeverity, ) - from phase_config import get_thinking_budget, resolve_model_id - from services.category_utils import map_category - from services.io_utils import safe_print - from services.pr_worktree_manager import PRWorktreeManager - from services.pydantic_models import ParallelFollowupResponse - from services.sdk_utils import process_sdk_stream + from runners.github.services.category_utils import map_category + from runners.github.services.io_utils import safe_print + from runners.github.services.pr_worktree_manager import PRWorktreeManager + from runners.github.services.pydantic_models import ParallelFollowupResponse + from runners.github.services.sdk_utils import process_sdk_stream logger = logging.getLogger(__name__) diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index aa4bf9479..99a554927 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -47,10 +47,15 @@ from .pydantic_models import AgentAgreement, ParallelOrchestratorResponse from .sdk_utils import process_sdk_stream except (ImportError, ValueError, SystemError): - from context_gatherer import PRContext, PRContextGatherer, _validate_git_ref from core.client import create_client - from gh_client import GHClient - from models import ( + from phase_config import get_thinking_budget, resolve_model_id + from runners.github.context_gatherer import ( + PRContext, + PRContextGatherer, + _validate_git_ref, + ) + from runners.github.gh_client import GHClient + from runners.github.models import ( BRANCH_BEHIND_BLOCKER_MSG, BRANCH_BEHIND_REASONING, GitHubRunnerConfig, @@ -59,12 +64,14 @@ PRReviewResult, ReviewSeverity, ) - from phase_config import get_thinking_budget, resolve_model_id - from services.category_utils import map_category - from services.io_utils import safe_print - from services.pr_worktree_manager import PRWorktreeManager - from services.pydantic_models import AgentAgreement, ParallelOrchestratorResponse - from services.sdk_utils import process_sdk_stream + from runners.github.services.category_utils import map_category + from runners.github.services.io_utils import safe_print + from runners.github.services.pr_worktree_manager import PRWorktreeManager + from runners.github.services.pydantic_models import ( + AgentAgreement, + ParallelOrchestratorResponse, + ) + from runners.github.services.sdk_utils import process_sdk_stream logger = logging.getLogger(__name__) diff --git a/apps/backend/runners/github/services/pr_review_engine.py b/apps/backend/runners/github/services/pr_review_engine.py index 867a8f191..8a74faca1 100644 --- a/apps/backend/runners/github/services/pr_review_engine.py +++ b/apps/backend/runners/github/services/pr_review_engine.py @@ -26,18 +26,18 @@ from .prompt_manager import PromptManager from .response_parsers import ResponseParser except (ImportError, ValueError, SystemError): - from context_gatherer import PRContext - from models import ( + from phase_config import resolve_model_id + from runners.github.context_gatherer import PRContext + from runners.github.models import ( AICommentTriage, GitHubRunnerConfig, PRReviewFinding, ReviewPass, StructuralIssue, ) - from phase_config import resolve_model_id - from services.io_utils import safe_print - from services.prompt_manager import PromptManager - from services.response_parsers import ResponseParser + from runners.github.services.io_utils import safe_print + from runners.github.services.prompt_manager import PromptManager + from runners.github.services.response_parsers import ResponseParser # Define a local ProgressCallback to avoid circular import diff --git a/apps/backend/runners/github/services/prompt_manager.py b/apps/backend/runners/github/services/prompt_manager.py index 882a8fe2f..849a19916 100644 --- a/apps/backend/runners/github/services/prompt_manager.py +++ b/apps/backend/runners/github/services/prompt_manager.py @@ -12,7 +12,7 @@ try: from ..models import ReviewPass except (ImportError, ValueError, SystemError): - from models import ReviewPass + from runners.github.models import ReviewPass class PromptManager: diff --git a/apps/backend/runners/github/services/response_parsers.py b/apps/backend/runners/github/services/response_parsers.py index 0a2462722..23d54f65c 100644 --- a/apps/backend/runners/github/services/response_parsers.py +++ b/apps/backend/runners/github/services/response_parsers.py @@ -23,7 +23,7 @@ ) from .io_utils import safe_print except (ImportError, ValueError, SystemError): - from models import ( + from runners.github.models import ( AICommentTriage, AICommentVerdict, PRReviewFinding, @@ -33,7 +33,7 @@ TriageCategory, TriageResult, ) - from services.io_utils import safe_print + from runners.github.services.io_utils import safe_print # Evidence-based validation replaces confidence scoring # Findings without evidence are filtered out instead of using confidence thresholds diff --git a/apps/backend/runners/github/services/review_tools.py b/apps/backend/runners/github/services/review_tools.py index 7ee2ca19e..7656eda02 100644 --- a/apps/backend/runners/github/services/review_tools.py +++ b/apps/backend/runners/github/services/review_tools.py @@ -23,9 +23,9 @@ except (ImportError, ValueError, SystemError): from analysis.test_discovery import TestDiscovery from category_utils import map_category - from context_gatherer import PRContext from core.client import create_client - from models import PRReviewFinding, ReviewSeverity + from runners.github.context_gatherer import PRContext + from runners.github.models import PRReviewFinding, ReviewSeverity logger = logging.getLogger(__name__) diff --git a/apps/backend/runners/github/services/triage_engine.py b/apps/backend/runners/github/services/triage_engine.py index de38370fb..0d5eb22fb 100644 --- a/apps/backend/runners/github/services/triage_engine.py +++ b/apps/backend/runners/github/services/triage_engine.py @@ -15,10 +15,10 @@ from .prompt_manager import PromptManager from .response_parsers import ResponseParser except (ImportError, ValueError, SystemError): - from models import GitHubRunnerConfig, TriageCategory, TriageResult from phase_config import resolve_model_id - from services.prompt_manager import PromptManager - from services.response_parsers import ResponseParser + from runners.github.models import GitHubRunnerConfig, TriageCategory, TriageResult + from runners.github.services.prompt_manager import PromptManager + from runners.github.services.response_parsers import ResponseParser class TriageEngine: diff --git a/apps/backend/tests/context/test_prioritization.py b/apps/backend/tests/context/test_prioritization.py index ad9ce5095..5240b7987 100644 --- a/apps/backend/tests/context/test_prioritization.py +++ b/apps/backend/tests/context/test_prioritization.py @@ -81,7 +81,9 @@ def sample_matches(): class TestPrioritizationIntegration: """Test suite for prioritization integration.""" - def test_builder_has_prioritization_components(self, temp_project, mock_project_index): + def test_builder_has_prioritization_components( + self, temp_project, mock_project_index + ): """Test that ContextBuilder initializes prioritization components.""" builder = ContextBuilder( project_dir=temp_project, diff --git a/apps/backend/tests/test_recovery_e2e.py b/apps/backend/tests/test_recovery_e2e.py index 38eeeda5d..fb1ee14c7 100644 --- a/apps/backend/tests/test_recovery_e2e.py +++ b/apps/backend/tests/test_recovery_e2e.py @@ -33,7 +33,12 @@ @pytest.fixture(autouse=True) def cleanup_recovery_state(): """Cleanup test data before and after each test.""" - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent recovery_manager = RecoveryManager(spec_dir, project_dir) @@ -41,7 +46,7 @@ def cleanup_recovery_state(): "test-subtask-e2e-1", "test-subtask-e2e-2", "test-subtask-e2e-3", - "test-subtask-e2e-circular" + "test-subtask-e2e-circular", ] # Cleanup before test @@ -67,7 +72,12 @@ def test_first_attempt(): print("TEST 1: First Attempt (No History)") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Get recovery context for first attempt @@ -79,7 +89,9 @@ def test_first_attempt(): print(f"✓ Recovery hints: {recovery_hints}") assert attempt_count == 0, f"Expected 0 attempts, got {attempt_count}" - assert recovery_hints is None, f"Expected None for first attempt, got {recovery_hints}" + assert recovery_hints is None, ( + f"Expected None for first attempt, got {recovery_hints}" + ) print("✅ TEST 1 PASSED: First attempt has no history\n") @@ -90,7 +102,12 @@ def test_record_attempt_and_retrieve(): print("TEST 2: Record Attempt and Retrieve Context") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Create recovery manager @@ -103,7 +120,7 @@ def test_record_attempt_and_retrieve(): session=1, success=False, approach="Tried implementing async await pattern", - error="SyntaxError: invalid syntax" + error="SyntaxError: invalid syntax", ) print(f"✓ Recorded failed attempt for {test_subtask_id}") @@ -131,7 +148,12 @@ def test_multiple_attempts_with_stuck_notification(): print("TEST 3: Multiple Attempts → Stuck Notification") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Create recovery manager @@ -142,8 +164,14 @@ def test_multiple_attempts_with_stuck_notification(): # Record 3 failed attempts approaches = [ ("Tried implementing async await pattern", "SyntaxError: invalid syntax"), - ("Attempted refactor to use callbacks", "TypeError: callback is not a function"), - ("Switched to Promise-based approach", "ReferenceError: Promise is not defined"), + ( + "Attempted refactor to use callbacks", + "TypeError: callback is not a function", + ), + ( + "Switched to Promise-based approach", + "ReferenceError: Promise is not defined", + ), ] for i, (approach, error) in enumerate(approaches, 1): @@ -152,14 +180,14 @@ def test_multiple_attempts_with_stuck_notification(): session=i, success=False, approach=approach, - error=error + error=error, ) print(f"✓ Recorded attempt {i}: {approach[:50]}...") # Mark subtask as stuck recovery_manager.mark_subtask_stuck( test_subtask_id, - "Multiple different approaches failed with syntax and type errors" + "Multiple different approaches failed with syntax and type errors", ) print(f"✓ Marked {test_subtask_id} as stuck") @@ -173,7 +201,7 @@ def test_multiple_attempts_with_stuck_notification(): subtask_id=test_subtask_id, reason="Multiple different approaches failed with syntax and type errors", attempt_count=3, - spec_dir=spec_dir + spec_dir=spec_dir, ) notification_output = sys.stdout.getvalue() @@ -187,15 +215,18 @@ def test_multiple_attempts_with_stuck_notification(): # Verify notification content assert test_subtask_id in notification_output, "Subtask ID not in notification" - assert "Manual Intervention Required" in notification_output, "Stuck message not in notification" + assert "Manual Intervention Required" in notification_output, ( + "Stuck message not in notification" + ) assert "3" in notification_output, "Attempt count not in notification" assert "spec" in notification_output.lower(), "Spec location not in notification" # Verify stuck state stuck_subtasks = recovery_manager.get_stuck_subtasks() assert len(stuck_subtasks) > 0, "No stuck subtasks recorded" - assert any(s["subtask_id"] == test_subtask_id for s in stuck_subtasks), \ + assert any(s["subtask_id"] == test_subtask_id for s in stuck_subtasks), ( f"Test subtask not in stuck list: {stuck_subtasks}" + ) print("✅ TEST 3 PASSED: Multiple attempts → Stuck notification triggered\n") @@ -206,7 +237,12 @@ def test_recovery_context_in_prompts(): print("TEST 4: Recovery Context Integration in Prompts") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Create recovery manager @@ -220,14 +256,14 @@ def test_recovery_context_in_prompts(): session=1, success=False, approach="Used Python asyncio library", - error="ImportError: No module named 'asyncio'" + error="ImportError: No module named 'asyncio'", ) recovery_manager.record_attempt( subtask_id=test_subtask_id, session=2, success=False, approach="Tried threading module instead", - error="RuntimeError: Thread deadlock detected" + error="RuntimeError: Thread deadlock detected", ) print(f"✓ Recorded 2 failed attempts for {test_subtask_id}") @@ -248,12 +284,13 @@ def test_recovery_context_in_prompts(): # Verify recovery hints contain useful information assert attempt_count == 2, f"Expected 2 attempts, got {attempt_count}" assert recovery_hints is not None, "Expected recovery hints" - assert any("Previous attempts" in h for h in recovery_hints), \ + assert any("Previous attempts" in h for h in recovery_hints), ( "Missing attempt count info in hints" - assert any("FAILED" in h for h in recovery_hints), \ - "Missing failure status in hints" - assert any("DIFFERENT approach" in h for h in recovery_hints), \ + ) + assert any("FAILED" in h for h in recovery_hints), "Missing failure status in hints" + assert any("DIFFERENT approach" in h for h in recovery_hints), ( "Missing guidance to try different approach" + ) print("\n✅ TEST 4 PASSED: Recovery context properly formatted for prompts\n") @@ -264,7 +301,12 @@ def test_metrics_reporting(): print("TEST 5: Recovery Metrics Reporting") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Get recovery metrics summary @@ -290,7 +332,7 @@ def test_metrics_reporting(): "failed_recoveries", "circular_fixes", "success_rate", - "avg_iterations" + "avg_iterations", ] found_keys = [k for k in possible_keys if k in metrics_summary] print(f"\n✓ Found {len(found_keys)} metric keys: {found_keys}") @@ -308,7 +350,12 @@ def test_circular_fix_detection(): print("TEST 6: Circular Fix Detection") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) project_dir = Path(__file__).parent.parent # Create recovery manager @@ -329,14 +376,13 @@ def test_circular_fix_detection(): session=i, success=False, approach=approach, - error=error + error=error, ) print(f"✓ Recorded attempt {i}: {approach[:50]}...") # Check if circular fix is detected is_circular = recovery_manager.is_circular_fix( - test_subtask_id, - "Using async await pattern again" + test_subtask_id, "Using async await pattern again" ) print(f"\n✓ Circular fix detected: {is_circular}") @@ -353,7 +399,12 @@ def cleanup_test_data(): print("CLEANUP: Removing Test Data") print("=" * 70) - spec_dir = Path(__file__).parent.parent / ".auto-claude" / "specs" / "064-intelligent-error-recovery" + spec_dir = ( + Path(__file__).parent.parent + / ".auto-claude" + / "specs" + / "064-intelligent-error-recovery" + ) memory_dir = spec_dir / "memory" # Reset test subtasks @@ -363,7 +414,7 @@ def cleanup_test_data(): "test-subtask-e2e-1", "test-subtask-e2e-2", "test-subtask-e2e-3", - "test-subtask-e2e-circular" + "test-subtask-e2e-circular", ] for subtask_id in test_subtasks: diff --git a/apps/frontend/src/main/claude-profile/usage-monitor.ts b/apps/frontend/src/main/claude-profile/usage-monitor.ts index 84a4852d5..1831b11e0 100644 --- a/apps/frontend/src/main/claude-profile/usage-monitor.ts +++ b/apps/frontend/src/main/claude-profile/usage-monitor.ts @@ -278,7 +278,7 @@ export class UsageMonitor extends EventEmitter { // Check immediately, then schedule next check with dynamic interval const scheduleNext = () => { const backoffMultiplier = Math.min( - Math.pow(2, this.consecutiveGlobalFailures), + 2 ** this.consecutiveGlobalFailures, UsageMonitor.MAX_BACKOFF_MULTIPLIER ); const nextInterval = baseInterval * backoffMultiplier; diff --git a/apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts index b031a3c48..60bea9063 100644 --- a/apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts @@ -8,6 +8,15 @@ import type { IPCResult, GitHubRepository, GitHubSyncStatus } from '../../../sha import { projectStore } from '../../project-store'; import { getGitHubConfig, githubFetch, normalizeRepoReference } from './utils'; import type { GitHubAPIRepository } from './types'; +import { + runPythonSubprocess, + getPythonPath, + getRunnerPath, + validateGitHubModule, + buildRunnerArgs, + parseJSONFromOutput, +} from './utils/subprocess-runner'; +import { getRunnerEnv } from './utils/runner-env'; /** * Check GitHub connection status @@ -132,10 +141,125 @@ export function registerGetRepositories(): void { ); } +/** + * Trigger code review for a repository + * Channel: github:code-review:trigger + */ +export function registerCodeReviewTrigger(): void { + ipcMain.handle( + IPC_CHANNELS.GITHUB_CODE_REVIEW_TRIGGER, + async (_, projectId: string, prNumber?: number): Promise> => { + const project = projectStore.getProject(projectId); + if (!project) { + return { success: false, error: 'Project not found' }; + } + + const config = getGitHubConfig(project); + if (!config) { + return { success: false, error: 'No GitHub token or repository configured' }; + } + + try { + // Normalize repo reference + const normalizedRepo = normalizeRepoReference(config.repo); + if (!normalizedRepo) { + return { + success: false, + error: 'Invalid repository format. Use owner/repo or GitHub URL.' + }; + } + + // Validate GitHub module is available + const validation = await validateGitHubModule(project); + if (!validation.valid) { + return { + success: false, + error: validation.error || 'GitHub runner not available' + }; + } + + // Generate a review ID for tracking + const reviewId = `review-${Date.now()}`; + + // Call backend code review service via Python runner + const backendPath = validation.backendPath!; + const pythonPath = getPythonPath(backendPath); + const runnerPath = getRunnerPath(backendPath); + + // Build command arguments + const args = buildRunnerArgs( + runnerPath, + project.path, + 'code-review-pr', + prNumber ? [String(prNumber)] : [] + ); + + // Get runner environment with authentication + const env = await getRunnerEnv(); + + // Execute the Python subprocess + const { promise } = runPythonSubprocess({ + pythonPath, + args, + cwd: project.path, + env, + onStdout: (line: string) => { + // Redact potential secrets from log output + const sanitized = line.replace( + /(token|key|secret|password|credential)[=: ]+\S+/gi, + '$1=[REDACTED]' + ); + console.log('[Code Review]', sanitized); + }, + onStderr: (line: string) => { + const sanitized = line.replace( + /(token|key|secret|password|credential)[=: ]+\S+/gi, + '$1=[REDACTED]' + ); + console.error('[Code Review Error]', sanitized); + }, + onComplete: (stdout: string, _stderr: string) => { + // Try to parse findings from output + try { + return parseJSONFromOutput(stdout); + } catch { + // If no JSON, return stdout as message (truncated for safety) + return { message: (stdout || '').slice(0, 2000) }; + } + }, + }); + + const result = await promise; + + if (!result.success) { + return { + success: false, + error: result.error || 'Code review failed' + }; + } + + return { + success: true, + data: { + reviewId, + findings: result.data?.findings || [], + } + }; + } catch (error) { + return { + success: false, + error: error instanceof Error ? error.message : 'Failed to trigger code review' + }; + } + } + ); +} + /** * Register all repository-related handlers */ export function registerRepositoryHandlers(): void { registerCheckConnection(); registerGetRepositories(); + registerCodeReviewTrigger(); } diff --git a/apps/frontend/src/renderer/components/CodeReview/ReviewPanel.tsx b/apps/frontend/src/renderer/components/CodeReview/ReviewPanel.tsx new file mode 100644 index 000000000..5ebcd1b6b --- /dev/null +++ b/apps/frontend/src/renderer/components/CodeReview/ReviewPanel.tsx @@ -0,0 +1,194 @@ +import { memo } from 'react'; +import { useTranslation } from 'react-i18next'; +import { Shield, AlertTriangle, CheckCircle, Loader2 } from 'lucide-react'; +import { Card, CardContent, CardHeader } from '../ui/card'; +import { Badge } from '../ui/badge'; +import { Button } from '../ui/button'; +import { cn } from '../../lib/utils'; + +interface ReviewFinding { + id: string; + severity: 'critical' | 'high' | 'medium' | 'low'; + category: 'security' | 'performance' | 'style' | 'bestPractices'; + message: string; + file?: string; + line?: number; +} + +interface ReviewPanelProps { + status?: 'idle' | 'analyzing' | 'complete' | 'failed'; + findings?: ReviewFinding[]; + onRunReview?: () => void; + onViewFindings?: () => void; +} + +// Custom comparator for React.memo - compares status, findings content, and callbacks +function reviewPanelPropsAreEqual(prevProps: ReviewPanelProps, nextProps: ReviewPanelProps): boolean { + if ( + prevProps.status !== nextProps.status || + prevProps.onRunReview !== nextProps.onRunReview || + prevProps.onViewFindings !== nextProps.onViewFindings + ) { + return false; + } + const prevFindings = prevProps.findings ?? []; + const nextFindings = nextProps.findings ?? []; + if (prevFindings.length !== nextFindings.length) return false; + // Shallow compare finding ids to detect content changes + return prevFindings.every((f, i) => f.id === nextFindings[i].id); +} + +export const ReviewPanel = memo(function ReviewPanel({ + status = 'idle', + findings = [], + onRunReview, + onViewFindings +}: ReviewPanelProps) { + const { t } = useTranslation(['codeReview', 'common']); + + const isAnalyzing = status === 'analyzing'; + const isComplete = status === 'complete'; + const findingsCount = findings.length; + + const getSeverityColor = (severity: ReviewFinding['severity']) => { + switch (severity) { + case 'critical': + return 'bg-red-500/10 text-red-400 border-red-500/30'; + case 'high': + return 'bg-orange-500/10 text-orange-400 border-orange-500/30'; + case 'medium': + return 'bg-yellow-500/10 text-yellow-400 border-yellow-500/30'; + case 'low': + return 'bg-blue-500/10 text-blue-400 border-blue-500/30'; + default: + return 'bg-muted text-muted-foreground border-border'; + } + }; + + const getStatusIcon = () => { + if (isAnalyzing) { + return ; + } + if (isComplete && findingsCount === 0) { + return ; + } + if (isComplete && findingsCount > 0) { + return ; + } + return ; + }; + + const getStatusLabel = () => { + if (isAnalyzing) { + return t('codeReview:reviewInProgress'); + } + if (isComplete) { + return t('codeReview:reviewComplete'); + } + return t('codeReview:status.idle'); + }; + + return ( + + +
+
+ {getStatusIcon()} +

+ {t('codeReview:title')} +

+
+ + {getStatusLabel()} + +
+ + + + {/* Empty state */} + {status === 'idle' && findingsCount === 0 && ( +
+

+ {t('codeReview:empty.description')} +

+ {onRunReview && ( + + )} +
+ )} + + {/* Analyzing state */} + {isAnalyzing && ( +
+ +

+ {t('codeReview:status.analyzing')} +

+
+ )} + + {/* Complete state with findings */} + {isComplete && findingsCount > 0 && ( +
+
+ + {t('codeReview:findingsCount', { count: findingsCount })} + + {onViewFindings && ( + + )} +
+ + {/* Findings summary */} +
+ {findings.slice(0, 3).map((finding) => ( + + {t(`codeReview:severity.${finding.severity}`)} + + ))} + {findingsCount > 3 && ( + + +{findingsCount - 3} + + )} +
+
+ )} + + {/* Complete state with no findings */} + {isComplete && findingsCount === 0 && ( +
+ +

+ {t('codeReview:noFindings')} +

+
+ )} + + + ); +}, reviewPanelPropsAreEqual); diff --git a/apps/frontend/src/shared/constants/ipc.ts b/apps/frontend/src/shared/constants/ipc.ts index 0258d4ec7..03cf33d09 100644 --- a/apps/frontend/src/shared/constants/ipc.ts +++ b/apps/frontend/src/shared/constants/ipc.ts @@ -460,6 +460,14 @@ export const IPC_CHANNELS = { GITHUB_TRIAGE_COMPLETE: 'github:triage:complete', GITHUB_TRIAGE_ERROR: 'github:triage:error', + // GitHub Code Review operations + GITHUB_CODE_REVIEW_TRIGGER: 'github:code-review:trigger', + + // GitHub Code Review events (main -> renderer) + GITHUB_CODE_REVIEW_PROGRESS: 'github:code-review:progress', + GITHUB_CODE_REVIEW_COMPLETE: 'github:code-review:complete', + GITHUB_CODE_REVIEW_ERROR: 'github:code-review:error', + // Merge Analytics operations MERGE_ANALYTICS_GET_HISTORY: 'mergeAnalytics:getHistory', MERGE_ANALYTICS_GET_SUMMARY: 'mergeAnalytics:getSummary', diff --git a/apps/frontend/src/shared/i18n/index.ts b/apps/frontend/src/shared/i18n/index.ts index 1c4e2bb90..207239fd6 100644 --- a/apps/frontend/src/shared/i18n/index.ts +++ b/apps/frontend/src/shared/i18n/index.ts @@ -13,6 +13,7 @@ import enGitlab from './locales/en/gitlab.json'; import enTaskReview from './locales/en/taskReview.json'; import enTerminal from './locales/en/terminal.json'; import enErrors from './locales/en/errors.json'; +import enCodeReview from './locales/en/codeReview.json'; import enQuality from './locales/en/quality.json'; // Import French translation resources @@ -27,6 +28,7 @@ import frGitlab from './locales/fr/gitlab.json'; import frTaskReview from './locales/fr/taskReview.json'; import frTerminal from './locales/fr/terminal.json'; import frErrors from './locales/fr/errors.json'; +import frCodeReview from './locales/fr/codeReview.json'; import frQuality from './locales/fr/quality.json'; export const defaultNS = 'common'; @@ -44,6 +46,7 @@ export const resources = { taskReview: enTaskReview, terminal: enTerminal, errors: enErrors, + codeReview: enCodeReview, quality: enQuality }, fr: { @@ -58,6 +61,7 @@ export const resources = { taskReview: frTaskReview, terminal: frTerminal, errors: frErrors, + codeReview: frCodeReview, quality: frQuality } } as const; @@ -69,7 +73,7 @@ i18n lng: 'en', // Default language (will be overridden by settings) fallbackLng: 'en', defaultNS, - ns: ['common', 'navigation', 'settings', 'tasks', 'welcome', 'onboarding', 'dialogs', 'gitlab', 'taskReview', 'terminal', 'errors', 'quality'], + ns: ['common', 'navigation', 'settings', 'tasks', 'welcome', 'onboarding', 'dialogs', 'gitlab', 'taskReview', 'terminal', 'errors', 'codeReview', 'quality'], interpolation: { escapeValue: false // React already escapes values }, diff --git a/apps/frontend/src/shared/i18n/locales/en/codeReview.json b/apps/frontend/src/shared/i18n/locales/en/codeReview.json new file mode 100644 index 000000000..ced908c06 --- /dev/null +++ b/apps/frontend/src/shared/i18n/locales/en/codeReview.json @@ -0,0 +1,36 @@ +{ + "title": "Code Review", + "runReview": "Run Code Review", + "reviewInProgress": "Review in Progress", + "reviewComplete": "Review Complete", + "noFindings": "No issues found", + "findingsCount_zero": "No findings", + "findingsCount_one": "{{count}} finding", + "findingsCount_other": "{{count}} findings", + "status": { + "idle": "Ready", + "analyzing": "Analyzing...", + "complete": "Complete", + "failed": "Failed" + }, + "categories": { + "security": "Security", + "performance": "Performance", + "style": "Style", + "bestPractices": "Best Practices" + }, + "severity": { + "critical": "Critical", + "high": "High", + "medium": "Medium", + "low": "Low" + }, + "actions": { + "viewFindings": "View Findings", + "dismiss": "Dismiss" + }, + "empty": { + "title": "No active review", + "description": "Trigger a code review to analyze changes" + } +} diff --git a/apps/frontend/src/shared/i18n/locales/fr/codeReview.json b/apps/frontend/src/shared/i18n/locales/fr/codeReview.json new file mode 100644 index 000000000..5f7572ceb --- /dev/null +++ b/apps/frontend/src/shared/i18n/locales/fr/codeReview.json @@ -0,0 +1,36 @@ +{ + "title": "Revue de Code", + "runReview": "Lancer la Revue", + "reviewInProgress": "Revue en Cours", + "reviewComplete": "Revue Terminée", + "noFindings": "Aucun problème trouvé", + "findingsCount_zero": "Aucun résultat", + "findingsCount_one": "{{count}} résultat", + "findingsCount_other": "{{count}} résultats", + "status": { + "idle": "Prêt", + "analyzing": "Analyse...", + "complete": "Terminé", + "failed": "Échec" + }, + "categories": { + "security": "Sécurité", + "performance": "Performance", + "style": "Style", + "bestPractices": "Bonnes Pratiques" + }, + "severity": { + "critical": "Critique", + "high": "Élevé", + "medium": "Moyen", + "low": "Faible" + }, + "actions": { + "viewFindings": "Voir les Résultats", + "dismiss": "Ignorer" + }, + "empty": { + "title": "Aucune revue active", + "description": "Lancez une revue de code pour analyser les changements" + } +} diff --git a/tests/agents/test_code_reviewer.py b/tests/agents/test_code_reviewer.py new file mode 100644 index 000000000..8f8fb049c --- /dev/null +++ b/tests/agents/test_code_reviewer.py @@ -0,0 +1,624 @@ +#!/usr/bin/env python3 +""" +Unit Tests for Code Review Agent +================================== + +Tests for the code review agent session covering: +- Successful review returning ("approved", response) +- Review with issues returning ("issues_found", response) +- Error handling returning ("error", error_message) +- Graphiti memory integration (context loading and saving) +- Report file creation and parsing +""" + +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +_BACKEND_PATH = str(Path(__file__).parent.parent / "apps" / "backend") +if _BACKEND_PATH not in sys.path: + sys.path.insert(0, _BACKEND_PATH) + + +# ============================================================================= +# TEST FIXTURES +# ============================================================================= + + +@pytest.fixture +def test_env(temp_git_repo: Path): + """ + Create a test environment using the shared temp_git_repo fixture. + + Yields: + tuple: (temp_dir, spec_dir, project_dir) + """ + temp_dir = temp_git_repo + spec_dir = temp_dir / "spec" + project_dir = temp_dir + + spec_dir.mkdir(parents=True, exist_ok=True) + + # Create spec.md + (spec_dir / "spec.md").write_text( + "# Test Spec\n\nTest feature for code review" + ) + + yield temp_dir, spec_dir, project_dir + + +@pytest.fixture +def mock_client(): + """Create a mock ClaudeSDKClient.""" + client = MagicMock() + client.create_agent_session = AsyncMock() + return client + + +# ============================================================================= +# SUCCESSFUL REVIEW TESTS +# ============================================================================= + + +class TestSuccessfulReview: + """Tests for successful code review scenarios.""" + + @pytest.mark.asyncio + async def test_approved_review_returns_approved_status( + self, test_env, mock_client + ): + """Test that approved review returns ('approved', response).""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + # Create review report indicating approval + report_content = """# Code Review Report + +## Status: APPROVED + +No security, performance, or style issues found. + +All checks passed. +""" + (spec_dir / "code_review_report.md").write_text(report_content) + + # Mock agent session to return success + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ( + "success", + "Review completed successfully", + ) + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert status == "approved", "Should return approved status" + assert "Review completed successfully" in response + + @pytest.mark.asyncio + async def test_approved_review_no_issues_variant(self, test_env, mock_client): + """Test approval detection with 'NO ISSUES' in report.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + report_content = """# Code Review Report + +## NO ISSUES FOUND + +The code review found no critical issues. +""" + (spec_dir / "code_review_report.md").write_text(report_content) + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert status == "approved", "Should detect approval from NO ISSUES" + + +# ============================================================================= +# ISSUES FOUND TESTS +# ============================================================================= + + +class TestIssuesFound: + """Tests for code review scenarios where issues are found.""" + + @pytest.mark.asyncio + async def test_review_with_issues_returns_issues_found( + self, test_env, mock_client + ): + """Test that review with issues returns ('issues_found', response).""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + # Create review report with issues + report_content = """# Code Review Report + +## Status: REJECTED + +Issues found: +- SQL injection vulnerability in user input +- Performance issue: N+1 query pattern +""" + (spec_dir / "code_review_report.md").write_text(report_content) + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review found issues") + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert ( + status == "issues_found" + ), "Should return issues_found status" + assert "Review found issues" in response + + @pytest.mark.asyncio + async def test_no_report_assumes_issues_found(self, test_env, mock_client): + """Test that missing report is treated as issues_found.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + # Don't create any report file + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Session completed") + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert ( + status == "issues_found" + ), "Should default to issues_found when no report" + + +# ============================================================================= +# ERROR HANDLING TESTS +# ============================================================================= + + +class TestErrorHandling: + """Tests for error handling in code review sessions.""" + + @pytest.mark.asyncio + async def test_session_error_returns_error_status( + self, test_env, mock_client + ): + """Test that session errors return ('error', error_message).""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("error", "API timeout") + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert status == "error", "Should return error status" + assert "API timeout" in response + + @pytest.mark.asyncio + async def test_exception_handling_returns_error( + self, test_env, mock_client + ): + """Test that exceptions are caught and returned as errors.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.side_effect = Exception("Unexpected error") + mock_context.return_value = None + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert status == "error", "Should catch exception and return error" + # Response should contain the exception type but not leak internal details + assert "Exception" in response + + +# ============================================================================= +# MEMORY INTEGRATION TESTS +# ============================================================================= + + +class TestMemoryIntegration: + """Tests for Graphiti memory integration.""" + + @pytest.mark.asyncio + async def test_memory_context_loading(self, test_env, mock_client): + """Test that Graphiti context is loaded before session.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + memory_context = """ +Previous security patterns: +- SQL injection found in user_controller.py +- XSS vulnerability in template rendering +""" + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_context.return_value = memory_context + mock_session.return_value = ("success", "Review completed") + + # Create approval report + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + # Verify context was loaded with correct arguments + mock_context.assert_called_once() + args, kwargs = mock_context.call_args + assert spec_dir in args, f"spec_dir not in call args: {args}" + assert project_dir in args, f"project_dir not in call args: {args}" + + # Verify context included in prompt passed to session + session_call = mock_session.call_args + assert "message" in session_call[1] + prompt = session_call[1]["message"] + assert "Previous security patterns" in prompt + + @pytest.mark.asyncio + async def test_memory_context_not_required(self, test_env, mock_client): + """Test that session works even if memory context is None.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_context.return_value = None # No memory available + mock_session.return_value = ("success", "Review completed") + + # Create approval report + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + status, response = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert status == "approved", "Should work without memory context" + + +# ============================================================================= +# REPORT FILE TESTS +# ============================================================================= + + +class TestReportFileParsing: + """Tests for review report file creation and parsing.""" + + @pytest.mark.asyncio + async def test_report_file_created_in_spec_dir(self, test_env, mock_client): + """Test that report file is created in correct location.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + # Simulate agent creating report + report_path = spec_dir / "code_review_report.md" + report_path.write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert report_path.exists(), "Report should be created in spec_dir" + + @pytest.mark.asyncio + async def test_report_parsing_case_insensitive( + self, test_env, mock_client + ): + """Test that report status parsing is case-insensitive.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + # Test lowercase + (spec_dir / "code_review_report.md").write_text( + "Status: approved\n\nAll good!" + ) + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + status, _ = await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=1, + ) + + assert ( + status == "approved" + ), "Should detect approval regardless of case" + + +# ============================================================================= +# SESSION PARAMETERS TESTS +# ============================================================================= + + +class TestSessionParameters: + """Tests for session parameter handling.""" + + @pytest.mark.asyncio + async def test_target_files_passed_to_prompt(self, test_env, mock_client): + """Test that target_files parameter is included in prompt.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + target_files = ["src/app.py", "src/utils.py"] + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + # Create approval report + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + target_files=target_files, + review_session=1, + ) + + # Verify target files in prompt + session_call = mock_session.call_args + prompt = session_call[1]["message"] + assert "src/app.py" in prompt + assert "src/utils.py" in prompt + + @pytest.mark.asyncio + async def test_pr_number_passed_to_prompt(self, test_env, mock_client): + """Test that pr_number parameter is included in prompt.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + # Create approval report + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + pr_number=123, + review_session=1, + ) + + # Verify PR number in prompt + session_call = mock_session.call_args + prompt = session_call[1]["message"] + assert "123" in prompt or "PR Number" in prompt + + @pytest.mark.asyncio + async def test_review_session_number_tracked(self, test_env, mock_client): + """Test that review_session parameter is tracked.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + # Create approval report + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + review_session=3, # Third iteration + ) + + # Verify session number in prompt + session_call = mock_session.call_args + prompt = session_call[1]["message"] + assert "3" in prompt or "Review Session" in prompt + + +# ============================================================================= +# SELF-CORRECTION TESTS +# ============================================================================= + + +class TestSelfCorrection: + """Tests for self-correction with previous error context.""" + + @pytest.mark.asyncio + async def test_previous_error_added_to_prompt(self, test_env, mock_client): + """Test that previous error context is included in prompt.""" + from agents.code_reviewer import run_code_review_session + + temp_dir, spec_dir, project_dir = test_env + + previous_error = { + "error_message": "Failed to create review report", + "error_type": "missing_report", + "consecutive_errors": 1, + } + + with patch( + "agents.code_reviewer.run_agent_session", new_callable=AsyncMock + ) as mock_session, patch( + "agents.code_reviewer.get_graphiti_context", + new_callable=AsyncMock, + ) as mock_context: + + mock_session.return_value = ("success", "Review completed") + mock_context.return_value = None + + # Create approval report this time + (spec_dir / "code_review_report.md").write_text("Status: APPROVED") + + await run_code_review_session( + client=mock_client, + project_dir=project_dir, + spec_dir=spec_dir, + previous_error=previous_error, + review_session=2, + ) + + # Verify error context in prompt + session_call = mock_session.call_args + prompt = session_call[1]["message"] + assert "PREVIOUS ITERATION FAILED" in prompt + assert "Failed to create review report" in prompt + assert "**Consecutive Failures**: 1" in prompt + + +# ============================================================================= +# MAIN ENTRY POINT +# ============================================================================= + + +def run_all_tests(): + """Run all tests using pytest.""" + sys.exit(pytest.main([__file__, "-v", "--tb=short"])) + + +if __name__ == "__main__": + run_all_tests() diff --git a/tests/runners/__init__.py b/tests/runners/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/runners/github/__init__.py b/tests/runners/github/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/runners/github/test_code_review_service.py b/tests/runners/github/test_code_review_service.py new file mode 100644 index 000000000..1c431665b --- /dev/null +++ b/tests/runners/github/test_code_review_service.py @@ -0,0 +1,666 @@ +#!/usr/bin/env python3 +""" +Integration Tests for Code Review Service +========================================== + +Tests for the CodeReviewService class covering: +- review_code_changes() method with mock SecurityScanner +- Security vulnerability to PR finding conversion +- Severity mapping (critical → CRITICAL, high → HIGH, etc.) +- post_review_to_github() with mock GHClient +- Markdown formatting of review body +- Review event determination (approve/comment/request-changes) +""" + +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Add backend to path for imports +sys.path.insert(0, str(Path(__file__).parent.parent.parent / "apps" / "backend")) + + +# ============================================================================= +# TEST FIXTURES +# ============================================================================= + + +@pytest.fixture +def test_env(temp_git_repo: Path): + """Create test environment with project and GitHub directories.""" + project_dir = temp_git_repo + github_dir = project_dir / ".github" / "automation" + github_dir.mkdir(parents=True, exist_ok=True) + + yield project_dir, github_dir + + +@pytest.fixture +def mock_config(): + """Create mock GitHubRunnerConfig.""" + from runners.github.models import GitHubRunnerConfig + + config = GitHubRunnerConfig( + token="test_token", + repo="test_owner/test_repo", + ) + return config + + +@pytest.fixture +def mock_security_scanner(): + """Create mock SecurityScanner.""" + scanner = MagicMock() + scanner.scan_project = MagicMock() + return scanner + + +@pytest.fixture +def sample_vulnerability(): + """Create a sample SecurityVulnerability.""" + from analysis.security_scanner import SecurityVulnerability + + return SecurityVulnerability( + title="SQL Injection Vulnerability", + description="Unsafe SQL query construction using user input", + severity="high", + file="app/database.py", + line=45, + cwe="CWE-89", + source="bandit", + ) + + +# ============================================================================= +# CODE REVIEW SERVICE INITIALIZATION TESTS +# ============================================================================= + + +class TestCodeReviewServiceInit: + """Tests for CodeReviewService initialization.""" + + def test_service_initialization(self, test_env, mock_config): + """Test that service initializes correctly.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + service = CodeReviewService( + project_dir=project_dir, + github_dir=github_dir, + config=mock_config, + ) + + assert service.project_dir == project_dir + assert service.github_dir == github_dir + assert service.config == mock_config + assert service.scanner is not None + + def test_service_with_progress_callback(self, test_env, mock_config): + """Test service initialization with progress callback.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + callback = MagicMock() + service = CodeReviewService( + project_dir=project_dir, + github_dir=github_dir, + config=mock_config, + progress_callback=callback, + ) + + assert service.progress_callback == callback + + +# ============================================================================= +# SEVERITY MAPPING TESTS +# ============================================================================= + + +class TestSeverityMapping: + """Tests for severity mapping from scanner to review severity.""" + + def test_critical_severity_mapping(self, test_env, mock_config): + """Test that 'critical' maps to ReviewSeverity.CRITICAL.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("critical") + assert result == ReviewSeverity.CRITICAL + + def test_high_severity_mapping(self, test_env, mock_config): + """Test that 'high' maps to ReviewSeverity.HIGH.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("high") + assert result == ReviewSeverity.HIGH + + def test_medium_severity_mapping(self, test_env, mock_config): + """Test that 'medium' maps to ReviewSeverity.MEDIUM.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("medium") + assert result == ReviewSeverity.MEDIUM + + def test_low_severity_mapping(self, test_env, mock_config): + """Test that 'low' maps to ReviewSeverity.LOW.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("low") + assert result == ReviewSeverity.LOW + + def test_info_severity_maps_to_low(self, test_env, mock_config): + """Test that 'info' maps to ReviewSeverity.LOW.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("info") + assert result == ReviewSeverity.LOW + + def test_unknown_severity_defaults_to_medium(self, test_env, mock_config): + """Test that unknown severity defaults to ReviewSeverity.MEDIUM.""" + from runners.github.models import ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_severity("unknown") + assert result == ReviewSeverity.MEDIUM + + +# ============================================================================= +# CATEGORY MAPPING TESTS +# ============================================================================= + + +class TestCategoryMapping: + """Tests for category mapping from scanner source to review category.""" + + def test_secrets_scanner_maps_to_security(self, test_env, mock_config): + """Test that 'secrets' scanner maps to ReviewCategory.SECURITY.""" + from runners.github.models import ReviewCategory + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_category("secrets") + assert result == ReviewCategory.SECURITY + + def test_bandit_scanner_maps_to_security(self, test_env, mock_config): + """Test that 'bandit' scanner maps to ReviewCategory.SECURITY.""" + from runners.github.models import ReviewCategory + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + result = service._map_category("bandit") + assert result == ReviewCategory.SECURITY + + +# ============================================================================= +# VULNERABILITY CONVERSION TESTS +# ============================================================================= + + +class TestVulnerabilityConversion: + """Tests for converting security vulnerabilities to PR findings.""" + + def test_convert_vulnerability_to_finding( + self, test_env, mock_config, sample_vulnerability + ): + """Test conversion of SecurityVulnerability to PRReviewFinding.""" + from runners.github.models import ReviewCategory, ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + finding = service._convert_vulnerability_to_finding( + sample_vulnerability + ) + + assert finding.title == "SQL Injection Vulnerability" + assert "Unsafe SQL query construction" in finding.description + assert finding.severity == ReviewSeverity.HIGH + assert finding.category == ReviewCategory.SECURITY + assert finding.file == "app/database.py" + assert finding.line == 45 + assert "CWE-89" in finding.description + + def test_finding_id_generation_is_unique( + self, test_env, mock_config, sample_vulnerability + ): + """Test that finding IDs are unique and consistent.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + finding1 = service._convert_vulnerability_to_finding( + sample_vulnerability + ) + finding2 = service._convert_vulnerability_to_finding( + sample_vulnerability + ) + + # Same vulnerability should produce same ID + assert finding1.id == finding2.id + + # ID should be a hash + assert len(finding1.id) == 12 + assert isinstance(finding1.id, str) + + def test_vulnerability_without_file_uses_project_wide( + self, test_env, mock_config + ): + """Test that vulnerability without file uses 'project-wide'.""" + from analysis.security_scanner import SecurityVulnerability + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + vuln = SecurityVulnerability( + title="Dependency Vulnerability", + description="Outdated package version", + severity="medium", + file=None, # No specific file + line=None, + cwe=None, + source="npm_audit", + ) + + finding = service._convert_vulnerability_to_finding(vuln) + + assert finding.file == "project-wide" + assert finding.line == 1 + + +# ============================================================================= +# SECRET DETECTION CONVERSION TESTS +# ============================================================================= + + +class TestSecretConversion: + """Tests for converting detected secrets to PR findings.""" + + def test_convert_secret_to_finding(self, test_env, mock_config): + """Test conversion of secret detection to PRReviewFinding.""" + from runners.github.models import ReviewCategory, ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + service = CodeReviewService(project_dir, github_dir, mock_config) + + secret = { + "file": "config/settings.py", + "line": 12, + "pattern": "AWS Access Key", + "match": "AKIA...", + } + + finding = service._convert_secret_to_finding(secret) + + assert "Detected secret: AWS Access Key" in finding.title + assert finding.severity == ReviewSeverity.CRITICAL # Always critical + assert finding.category == ReviewCategory.SECURITY + assert finding.file == "config/settings.py" + assert finding.line == 12 + + +# ============================================================================= +# REVIEW_CODE_CHANGES TESTS +# ============================================================================= + + +class TestReviewCodeChanges: + """Tests for review_code_changes method.""" + + @pytest.mark.asyncio + async def test_review_code_changes_with_vulnerabilities( + self, test_env, mock_config, sample_vulnerability + ): + """Test review_code_changes integrates SecurityScanner correctly.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + # Mock PRContext + mock_context = MagicMock() + mock_context.pr_number = 123 + mock_context.changed_files = ["app/database.py"] + + with patch( + "runners.github.services.code_review_service.SecurityScanner" + ) as MockScanner: + mock_scanner_instance = MagicMock() + # Mock scan() method (not scan_project) + mock_scan_result = MagicMock() + mock_scan_result.vulnerabilities = [sample_vulnerability] + mock_scan_result.secrets = [] + mock_scan_result.errors = [] + mock_scanner_instance.scan = MagicMock(return_value=mock_scan_result) + MockScanner.return_value = mock_scanner_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + + findings = await service.review_code_changes(mock_context) + + assert len(findings) == 1 + assert findings[0].title == "SQL Injection Vulnerability" + assert findings[0].file == "app/database.py" + + @pytest.mark.asyncio + async def test_review_code_changes_no_vulnerabilities( + self, test_env, mock_config + ): + """Test review_code_changes with no vulnerabilities found.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + mock_context = MagicMock() + mock_context.pr_number = 123 + mock_context.changed_files = ["app/clean.py"] + + with patch( + "runners.github.services.code_review_service.SecurityScanner" + ) as MockScanner: + mock_scanner_instance = MagicMock() + mock_scanner_instance.scan_project = MagicMock(return_value=[]) + MockScanner.return_value = mock_scanner_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + + findings = await service.review_code_changes(mock_context) + + assert len(findings) == 0 + + +# ============================================================================= +# POST_REVIEW_TO_GITHUB TESTS +# ============================================================================= + + +class TestPostReviewToGitHub: + """Tests for post_review_to_github method.""" + + @pytest.mark.asyncio + async def test_post_review_with_critical_findings( + self, test_env, mock_config + ): + """Test that critical findings result in REQUEST_CHANGES event.""" + from runners.github.models import PRReviewFinding, ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + critical_finding = PRReviewFinding( + id="test123", + severity=ReviewSeverity.CRITICAL, + category="SECURITY", + title="Critical Security Issue", + description="This is critical", + file="app/auth.py", + line=10, + ) + + mock_context = MagicMock() + mock_context.pr_number = 123 + + with patch( + "runners.github.services.code_review_service.GHClient" + ) as MockGHClient: + mock_gh_instance = MagicMock() + mock_gh_instance.pr_review = AsyncMock() + MockGHClient.return_value = mock_gh_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + service.gh_client = mock_gh_instance + + await service.post_review_to_github( + mock_context, [critical_finding] + ) + + # Verify review was posted + mock_gh_instance.pr_review.assert_called_once() + call_args = mock_gh_instance.pr_review.call_args + + # Should be request-changes for critical issues + assert call_args[1].get("event") in [ + "REQUEST_CHANGES", + "request_changes", + "request-changes", + ] + + @pytest.mark.asyncio + async def test_post_review_with_low_findings(self, test_env, mock_config): + """Test that low severity findings result in COMMENT event.""" + from runners.github.models import PRReviewFinding, ReviewSeverity + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + low_finding = PRReviewFinding( + id="test456", + severity=ReviewSeverity.LOW, + category="STYLE", + title="Minor Style Issue", + description="Consider renaming", + file="app/utils.py", + line=20, + ) + + mock_context = MagicMock() + mock_context.pr_number = 123 + + with patch( + "runners.github.services.code_review_service.GHClient" + ) as MockGHClient: + mock_gh_instance = MagicMock() + mock_gh_instance.pr_review = AsyncMock() + MockGHClient.return_value = mock_gh_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + service.gh_client = mock_gh_instance + + await service.post_review_to_github(mock_context, [low_finding]) + + # Verify review was posted + mock_gh_instance.pr_review.assert_called_once() + + @pytest.mark.asyncio + async def test_post_review_no_findings_approves( + self, test_env, mock_config + ): + """Test that no findings results in APPROVE event.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + mock_context = MagicMock() + mock_context.pr_number = 123 + + with patch( + "runners.github.services.code_review_service.GHClient" + ) as MockGHClient: + mock_gh_instance = MagicMock() + mock_gh_instance.pr_review = AsyncMock() + MockGHClient.return_value = mock_gh_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + service.gh_client = mock_gh_instance + + await service.post_review_to_github(mock_context, []) + + # Verify approval was posted + mock_gh_instance.pr_review.assert_called_once() + call_args = mock_gh_instance.pr_review.call_args + + # Should be APPROVE for no issues + assert "APPROVE" in str(call_args) or call_args[1].get( + "event" + ) in ["APPROVE", "approve"] + + +# ============================================================================= +# MARKDOWN FORMATTING TESTS +# ============================================================================= + + +class TestMarkdownFormatting: + """Tests for markdown formatting of review body.""" + + @pytest.mark.asyncio + async def test_review_body_markdown_format(self, test_env, mock_config): + """Test that review body is formatted as markdown.""" + from runners.github.models import ( + PRReviewFinding, + ReviewCategory, + ReviewSeverity, + ) + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + finding = PRReviewFinding( + id="md_test", + severity=ReviewSeverity.HIGH, + category=ReviewCategory.SECURITY, + title="Test Finding", + description="Test description", + file="test.py", + line=1, + ) + + mock_context = MagicMock() + mock_context.pr_number = 123 + + with patch( + "runners.github.services.code_review_service.GHClient" + ) as MockGHClient: + mock_gh_instance = MagicMock() + mock_gh_instance.pr_review = AsyncMock() + MockGHClient.return_value = mock_gh_instance + + service = CodeReviewService(project_dir, github_dir, mock_config) + service.gh_client = mock_gh_instance + + await service.post_review_to_github(mock_context, [finding]) + + # Verify markdown formatting in call + call_args = mock_gh_instance.pr_review.call_args + body = call_args[1].get("body", "") + + # Should contain markdown elements + assert isinstance(body, str) + # Markdown typically uses headers, lists, or bold text + # At minimum should mention the finding + assert len(body) > 0 + + +# ============================================================================= +# PROGRESS CALLBACK TESTS +# ============================================================================= + + +class TestProgressCallback: + """Tests for progress callback integration.""" + + @pytest.mark.asyncio + async def test_progress_callback_invoked(self, test_env, mock_config): + """Test that progress callback is invoked during review.""" + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + project_dir, github_dir = test_env + + callback = MagicMock() + service = CodeReviewService( + project_dir, github_dir, mock_config, progress_callback=callback + ) + + # Manually invoke progress report + service._report_progress("testing", 50, "Test message") + + # Verify callback was called + assert callback.called + + +# ============================================================================= +# MAIN ENTRY POINT +# ============================================================================= + + +def run_all_tests(): + """Run all tests using pytest.""" + sys.exit(pytest.main([__file__, "-v", "--tb=short"])) + + +if __name__ == "__main__": + run_all_tests() diff --git a/tests/runners/github/test_import_chain.py b/tests/runners/github/test_import_chain.py new file mode 100644 index 000000000..ea80f2853 --- /dev/null +++ b/tests/runners/github/test_import_chain.py @@ -0,0 +1,169 @@ +#!/usr/bin/env python3 +""" +Import Chain Verification Test +================================ + +Verifies that CodeReviewService can be imported correctly within +the GitHub runner context. The service is designed to be used within +the runner.py context, not standalone. +""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +# ============================================================================= +# IMPORT CHAIN TESTS +# ============================================================================= + + +class TestImportChain: + """Tests for verifying import chain works in GitHub runner context.""" + + def test_code_review_service_imports_in_runner_context(self): + """Test that CodeReviewService imports when sys.path includes runner context.""" + # Get the backend directory + backend_dir = ( + Path(__file__).parent.parent.parent / "apps" / "backend" + ) + + # Import should work when we add backend to sys.path + original_path = sys.path.copy() + try: + sys.path.insert(0, str(backend_dir)) + + # This should work in runner context + from runners.github.services.code_review_service import ( + CodeReviewService, + ) + + assert ( + CodeReviewService is not None + ), "CodeReviewService should import successfully" + + finally: + sys.path = original_path + + def test_runner_command_help_works(self): + """Test that the code-review-pr command help works.""" + backend_dir = ( + Path(__file__).parent.parent.parent / "apps" / "backend" + ) + runner_script = backend_dir / "runners" / "github" / "runner.py" + + # Skip test if runner script doesn't exist + if not runner_script.exists(): + pytest.skip("runner.py not found") + + # Try to run the help command + result = subprocess.run( + [sys.executable, str(runner_script), "code-review-pr", "--help"], + cwd=backend_dir, + capture_output=True, + text=True, + timeout=10, + ) + + # The command should either: + # 1. Show help (exit code 0) + # 2. Fail with a known error (but not import error) + # We just want to verify the module can be loaded + + # Check that it's not an import error + assert ( + "ImportError" not in result.stderr + ), f"Import error detected: {result.stderr}" + assert ( + "ModuleNotFoundError" not in result.stderr + ), f"Module not found: {result.stderr}" + + def test_service_dependencies_available(self): + """Test that service dependencies can be imported.""" + backend_dir = ( + Path(__file__).parent.parent.parent / "apps" / "backend" + ) + + original_path = sys.path.copy() + try: + sys.path.insert(0, str(backend_dir)) + + # Import dependencies that CodeReviewService needs + from analysis.security_scanner import SecurityScanner + from runners.github.models import GitHubRunnerConfig, PRReviewFinding + + assert SecurityScanner is not None + assert GitHubRunnerConfig is not None + assert PRReviewFinding is not None + + finally: + sys.path = original_path + + def test_import_from_runner_py_context(self): + """Test importing as if we're inside runner.py.""" + backend_dir = ( + Path(__file__).parent.parent.parent / "apps" / "backend" + ) + runners_dir = backend_dir / "runners" / "github" + + original_path = sys.path.copy() + + try: + # Simulate being in the runner.py context + sys.path.insert(0, str(backend_dir)) + sys.path.insert(0, str(runners_dir)) + + # Use absolute import path + from runners.github.services.code_review_service import CodeReviewService + + assert CodeReviewService is not None + + finally: + sys.path = original_path + + +# ============================================================================= +# DOCUMENTATION TEST +# ============================================================================= + + +class TestServiceDocumentation: + """Tests for service documentation.""" + + def test_service_has_usage_documentation(self): + """Test that service file has usage documentation.""" + backend_dir = ( + Path(__file__).parent.parent.parent / "apps" / "backend" + ) + service_file = ( + backend_dir / "runners" / "github" / "services" / "code_review_service.py" + ) + + if not service_file.exists(): + pytest.skip("code_review_service.py not found") + + content = service_file.read_text() + + # Should have documentation about usage + assert "Usage:" in content or "usage:" in content.lower() + + # Should document the import pattern + assert ( + "from runners.github.services.code_review_service import" in content + or "CodeReviewService" in content + ) + + +# ============================================================================= +# MAIN ENTRY POINT +# ============================================================================= + + +def run_all_tests(): + """Run all tests using pytest.""" + sys.exit(pytest.main([__file__, "-v", "--tb=short"])) + + +if __name__ == "__main__": + run_all_tests()