diff --git a/.codex/skills-index.json b/.codex/skills-index.json index 009e375..86fdb96 100644 --- a/.codex/skills-index.json +++ b/.codex/skills-index.json @@ -27,7 +27,7 @@ "name": "code-reviewer", "source": "../../engineering-team/code-reviewer", "category": "engineering", - "description": "Comprehensive code review skill for TypeScript, JavaScript, Python, Swift, Kotlin, Go. Includes automated code analysis, best practice checking, security scanning, and review checklist generation. Use when reviewing pull requests, providing code feedback, identifying issues, or ensuring code quality standards." + "description": "Code review automation for TypeScript, JavaScript, Python, Go, Swift, Kotlin. Analyzes PRs for complexity and risk, checks code quality for SOLID violations and code smells, generates review reports. Use when reviewing pull requests, analyzing code quality, identifying issues, generating review checklists." }, { "name": "ms365-tenant-manager", @@ -75,7 +75,7 @@ "name": "senior-frontend", "source": "../../engineering-team/senior-frontend", "category": "engineering", - "description": "Comprehensive frontend development skill for building modern, performant web applications using ReactJS, NextJS, TypeScript, Tailwind CSS. Includes component scaffolding, performance optimization, bundle analysis, and UI best practices. Use when developing frontend features, optimizing performance, implementing UI/UX designs, managing state, or reviewing frontend code." + "description": "Frontend development skill for React, Next.js, TypeScript, and Tailwind CSS applications. Use when building React components, optimizing Next.js performance, analyzing bundle sizes, scaffolding frontend projects, implementing accessibility, or reviewing frontend code quality." }, { "name": "senior-fullstack", @@ -123,7 +123,7 @@ "name": "tech-stack-evaluator", "source": "../../engineering-team/tech-stack-evaluator", "category": "engineering", - "description": "Comprehensive technology stack evaluation and comparison tool with TCO analysis, security assessment, and intelligent recommendations for engineering teams" + "description": "Technology stack evaluation and comparison with TCO analysis, security assessment, and ecosystem health scoring. Use when comparing frameworks, evaluating technology stacks, calculating total cost of ownership, assessing migration paths, or analyzing ecosystem viability." }, { "name": "app-store-optimization", @@ -195,7 +195,7 @@ "name": "fda-consultant-specialist", "source": "../../ra-qm-team/fda-consultant-specialist", "category": "ra-qm", - "description": "Senior FDA consultant and specialist for medical device companies including HIPAA compliance and requirement management. Provides FDA pathway expertise, QSR compliance, cybersecurity guidance, and regulatory submission support. Use for FDA submission planning, QSR compliance assessments, HIPAA evaluations, and FDA regulatory strategy development." + "description": "FDA regulatory consultant for medical device companies. Provides 510(k)/PMA/De Novo pathway guidance, QSR (21 CFR 820) compliance, HIPAA assessments, and device cybersecurity. Use when user mentions FDA submission, 510(k), PMA, De Novo, QSR, premarket, predicate device, substantial equivalence, HIPAA medical device, or FDA cybersecurity." }, { "name": "gdpr-dsgvo-expert", diff --git a/engineering-team/code-reviewer/SKILL.md b/engineering-team/code-reviewer/SKILL.md index ad7b451..3328ac1 100644 --- a/engineering-team/code-reviewer/SKILL.md +++ b/engineering-team/code-reviewer/SKILL.md @@ -1,209 +1,177 @@ --- name: code-reviewer -description: Comprehensive code review skill for TypeScript, JavaScript, Python, Swift, Kotlin, Go. Includes automated code analysis, best practice checking, security scanning, and review checklist generation. Use when reviewing pull requests, providing code feedback, identifying issues, or ensuring code quality standards. +description: Code review automation for TypeScript, JavaScript, Python, Go, Swift, Kotlin. Analyzes PRs for complexity and risk, checks code quality for SOLID violations and code smells, generates review reports. Use when reviewing pull requests, analyzing code quality, identifying issues, generating review checklists. --- # Code Reviewer -Complete toolkit for code reviewer with modern tools and best practices. +Automated code review tools for analyzing pull requests, detecting code quality issues, and generating review reports. -## Quick Start +--- -### Main Capabilities +## Table of Contents -This skill provides three core capabilities through automated scripts: +- [Tools](#tools) + - [PR Analyzer](#pr-analyzer) + - [Code Quality Checker](#code-quality-checker) + - [Review Report Generator](#review-report-generator) +- [Reference Guides](#reference-guides) +- [Languages Supported](#languages-supported) + +--- + +## Tools + +### PR Analyzer + +Analyzes git diff between branches to assess review complexity and identify risks. ```bash -# Script 1: Pr Analyzer -python scripts/pr_analyzer.py [options] +# Analyze current branch against main +python scripts/pr_analyzer.py /path/to/repo -# Script 2: Code Quality Checker -python scripts/code_quality_checker.py [options] +# Compare specific branches +python scripts/pr_analyzer.py . --base main --head feature-branch -# Script 3: Review Report Generator -python scripts/review_report_generator.py [options] +# JSON output for integration +python scripts/pr_analyzer.py /path/to/repo --json ``` -## Core Capabilities +**What it detects:** +- Hardcoded secrets (passwords, API keys, tokens) +- SQL injection patterns (string concatenation in queries) +- Debug statements (debugger, console.log) +- ESLint rule disabling +- TypeScript `any` types +- TODO/FIXME comments -### 1. Pr Analyzer +**Output includes:** +- Complexity score (1-10) +- Risk categorization (critical, high, medium, low) +- File prioritization for review order +- Commit message validation -Automated tool for pr analyzer tasks. +--- -**Features:** -- Automated scaffolding -- Best practices built-in -- Configurable templates -- Quality checks +### Code Quality Checker + +Analyzes source code for structural issues, code smells, and SOLID violations. -**Usage:** ```bash -python scripts/pr_analyzer.py [options] +# Analyze a directory +python scripts/code_quality_checker.py /path/to/code + +# Analyze specific language +python scripts/code_quality_checker.py . --language python + +# JSON output +python scripts/code_quality_checker.py /path/to/code --json ``` -### 2. Code Quality Checker +**What it detects:** +- Long functions (>50 lines) +- Large files (>500 lines) +- God classes (>20 methods) +- Deep nesting (>4 levels) +- Too many parameters (>5) +- High cyclomatic complexity +- Missing error handling +- Unused imports +- Magic numbers -Comprehensive analysis and optimization tool. +**Thresholds:** -**Features:** -- Deep analysis -- Performance metrics -- Recommendations -- Automated fixes +| Issue | Threshold | +|-------|-----------| +| Long function | >50 lines | +| Large file | >500 lines | +| God class | >20 methods | +| Too many params | >5 | +| Deep nesting | >4 levels | +| High complexity | >10 branches | + +--- + +### Review Report Generator + +Combines PR analysis and code quality findings into structured review reports. -**Usage:** ```bash -python scripts/code_quality_checker.py [--verbose] +# Generate report for current repo +python scripts/review_report_generator.py /path/to/repo + +# Markdown output +python scripts/review_report_generator.py . --format markdown --output review.md + +# Use pre-computed analyses +python scripts/review_report_generator.py . \ + --pr-analysis pr_results.json \ + --quality-analysis quality_results.json ``` -### 3. Review Report Generator +**Report includes:** +- Review verdict (approve, request changes, block) +- Score (0-100) +- Prioritized action items +- Issue summary by severity +- Suggested review order -Advanced tooling for specialized tasks. +**Verdicts:** -**Features:** -- Expert-level automation -- Custom configurations -- Integration ready -- Production-grade output +| Score | Verdict | +|-------|---------| +| 90+ with no high issues | Approve | +| 75+ with ≤2 high issues | Approve with suggestions | +| 50-74 | Request changes | +| <50 or critical issues | Block | -**Usage:** -```bash -python scripts/review_report_generator.py [arguments] [options] -``` +--- -## Reference Documentation +## Reference Guides ### Code Review Checklist +`references/code_review_checklist.md` -Comprehensive guide available in `references/code_review_checklist.md`: - -- Detailed patterns and practices -- Code examples -- Best practices -- Anti-patterns to avoid -- Real-world scenarios +Systematic checklists covering: +- Pre-review checks (build, tests, PR hygiene) +- Correctness (logic, data handling, error handling) +- Security (input validation, injection prevention) +- Performance (efficiency, caching, scalability) +- Maintainability (code quality, naming, structure) +- Testing (coverage, quality, mocking) +- Language-specific checks ### Coding Standards +`references/coding_standards.md` -Complete workflow documentation in `references/coding_standards.md`: - -- Step-by-step processes -- Optimization strategies -- Tool integrations -- Performance tuning -- Troubleshooting guide +Language-specific standards for: +- TypeScript (type annotations, null safety, async/await) +- JavaScript (declarations, patterns, modules) +- Python (type hints, exceptions, class design) +- Go (error handling, structs, concurrency) +- Swift (optionals, protocols, errors) +- Kotlin (null safety, data classes, coroutines) ### Common Antipatterns +`references/common_antipatterns.md` -Technical reference guide in `references/common_antipatterns.md`: +Antipattern catalog with examples and fixes: +- Structural (god class, long method, deep nesting) +- Logic (boolean blindness, stringly typed code) +- Security (SQL injection, hardcoded credentials) +- Performance (N+1 queries, unbounded collections) +- Testing (duplication, testing implementation) +- Async (floating promises, callback hell) -- Technology stack details -- Configuration examples -- Integration patterns -- Security considerations -- Scalability guidelines +--- -## Tech Stack +## Languages Supported -**Languages:** TypeScript, JavaScript, Python, Go, Swift, Kotlin -**Frontend:** React, Next.js, React Native, Flutter -**Backend:** Node.js, Express, GraphQL, REST APIs -**Database:** PostgreSQL, Prisma, NeonDB, Supabase -**DevOps:** Docker, Kubernetes, Terraform, GitHub Actions, CircleCI -**Cloud:** AWS, GCP, Azure - -## Development Workflow - -### 1. Setup and Configuration - -```bash -# Install dependencies -npm install -# or -pip install -r requirements.txt - -# Configure environment -cp .env.example .env -``` - -### 2. Run Quality Checks - -```bash -# Use the analyzer script -python scripts/code_quality_checker.py . - -# Review recommendations -# Apply fixes -``` - -### 3. Implement Best Practices - -Follow the patterns and practices documented in: -- `references/code_review_checklist.md` -- `references/coding_standards.md` -- `references/common_antipatterns.md` - -## Best Practices Summary - -### Code Quality -- Follow established patterns -- Write comprehensive tests -- Document decisions -- Review regularly - -### Performance -- Measure before optimizing -- Use appropriate caching -- Optimize critical paths -- Monitor in production - -### Security -- Validate all inputs -- Use parameterized queries -- Implement proper authentication -- Keep dependencies updated - -### Maintainability -- Write clear code -- Use consistent naming -- Add helpful comments -- Keep it simple - -## Common Commands - -```bash -# Development -npm run dev -npm run build -npm run test -npm run lint - -# Analysis -python scripts/code_quality_checker.py . -python scripts/review_report_generator.py --analyze - -# Deployment -docker build -t app:latest . -docker-compose up -d -kubectl apply -f k8s/ -``` - -## Troubleshooting - -### Common Issues - -Check the comprehensive troubleshooting section in `references/common_antipatterns.md`. - -### Getting Help - -- Review reference documentation -- Check script output messages -- Consult tech stack documentation -- Review error logs - -## Resources - -- Pattern Reference: `references/code_review_checklist.md` -- Workflow Guide: `references/coding_standards.md` -- Technical Guide: `references/common_antipatterns.md` -- Tool Scripts: `scripts/` directory +| Language | Extensions | +|----------|------------| +| Python | `.py` | +| TypeScript | `.ts`, `.tsx` | +| JavaScript | `.js`, `.jsx`, `.mjs` | +| Go | `.go` | +| Swift | `.swift` | +| Kotlin | `.kt`, `.kts` | diff --git a/engineering-team/code-reviewer/references/code_review_checklist.md b/engineering-team/code-reviewer/references/code_review_checklist.md index 30a0f7a..b7bd086 100644 --- a/engineering-team/code-reviewer/references/code_review_checklist.md +++ b/engineering-team/code-reviewer/references/code_review_checklist.md @@ -1,103 +1,270 @@ # Code Review Checklist -## Overview +Structured checklists for systematic code review across different aspects. -This reference guide provides comprehensive information for code reviewer. +--- -## Patterns and Practices +## Table of Contents -### Pattern 1: Best Practice Implementation +- [Pre-Review Checks](#pre-review-checks) +- [Correctness](#correctness) +- [Security](#security) +- [Performance](#performance) +- [Maintainability](#maintainability) +- [Testing](#testing) +- [Documentation](#documentation) +- [Language-Specific Checks](#language-specific-checks) -**Description:** -Detailed explanation of the pattern. +--- -**When to Use:** -- Scenario 1 -- Scenario 2 -- Scenario 3 +## Pre-Review Checks -**Implementation:** -```typescript -// Example code implementation -export class Example { - // Implementation details -} -``` +Before diving into code, verify these basics: -**Benefits:** -- Benefit 1 -- Benefit 2 -- Benefit 3 +### Build and Tests +- [ ] Code compiles without errors +- [ ] All existing tests pass +- [ ] New tests are included for new functionality +- [ ] No unintended files included (build artifacts, IDE configs) -**Trade-offs:** -- Consider 1 -- Consider 2 -- Consider 3 +### PR Hygiene +- [ ] PR has clear title and description +- [ ] Changes are scoped appropriately (not too large) +- [ ] Commits follow conventional commit format +- [ ] Branch is up to date with base branch -### Pattern 2: Advanced Technique +### Scope Verification +- [ ] Changes match the stated purpose +- [ ] No unrelated changes bundled in +- [ ] Breaking changes are documented +- [ ] Migration path provided if needed -**Description:** -Another important pattern for code reviewer. +--- -**Implementation:** -```typescript -// Advanced example -async function advancedExample() { - // Code here -} -``` +## Correctness -## Guidelines +### Logic +- [ ] Algorithm implements requirements correctly +- [ ] Edge cases handled (null, empty, boundary values) +- [ ] Off-by-one errors checked +- [ ] Correct operators used (== vs ===, & vs &&) +- [ ] Loop termination conditions correct +- [ ] Recursion has proper base cases -### Code Organization -- Clear structure -- Logical separation -- Consistent naming -- Proper documentation +### Data Handling +- [ ] Data types appropriate for the use case +- [ ] Numeric overflow/underflow considered +- [ ] Date/time handling accounts for timezones +- [ ] Unicode and internationalization handled +- [ ] Data validation at entry points -### Performance Considerations -- Optimization strategies -- Bottleneck identification -- Monitoring approaches -- Scaling techniques +### State Management +- [ ] State transitions are valid +- [ ] Race conditions addressed +- [ ] Concurrent access handled correctly +- [ ] State cleanup on errors/exit -### Security Best Practices -- Input validation -- Authentication -- Authorization -- Data protection +### Error Handling +- [ ] Errors caught at appropriate levels +- [ ] Error messages are actionable +- [ ] Errors don't expose sensitive information +- [ ] Recovery or graceful degradation implemented +- [ ] Resources cleaned up in error paths -## Common Patterns +--- -### Pattern A -Implementation details and examples. +## Security -### Pattern B -Implementation details and examples. +### Input Validation +- [ ] All user input validated and sanitized +- [ ] Input length limits enforced +- [ ] File uploads validated (type, size, content) +- [ ] URL parameters validated -### Pattern C -Implementation details and examples. +### Injection Prevention +- [ ] SQL queries parameterized +- [ ] Command execution uses safe APIs +- [ ] HTML output escaped to prevent XSS +- [ ] LDAP queries properly escaped +- [ ] XML parsing disables external entities -## Anti-Patterns to Avoid +### Authentication & Authorization +- [ ] Authentication required for protected resources +- [ ] Authorization checked before operations +- [ ] Session management secure +- [ ] Password handling follows best practices +- [ ] Token expiration implemented -### Anti-Pattern 1 -What not to do and why. +### Data Protection +- [ ] Sensitive data encrypted at rest +- [ ] Sensitive data encrypted in transit +- [ ] PII handled according to policy +- [ ] Secrets not hardcoded +- [ ] Logs don't contain sensitive data -### Anti-Pattern 2 -What not to do and why. +### API Security +- [ ] Rate limiting implemented +- [ ] CORS configured correctly +- [ ] CSRF protection in place +- [ ] API keys/tokens secured +- [ ] Endpoints use HTTPS -## Tools and Resources +--- -### Recommended Tools -- Tool 1: Purpose -- Tool 2: Purpose -- Tool 3: Purpose +## Performance -### Further Reading -- Resource 1 -- Resource 2 -- Resource 3 +### Efficiency +- [ ] Appropriate data structures used +- [ ] Algorithms have acceptable complexity +- [ ] Database queries are optimized +- [ ] N+1 query problems avoided +- [ ] Indexes used where beneficial -## Conclusion +### Resource Usage +- [ ] Memory usage bounded +- [ ] No memory leaks +- [ ] File handles properly closed +- [ ] Database connections pooled +- [ ] Network calls minimized -Key takeaways for using this reference guide effectively. +### Caching +- [ ] Appropriate caching strategy +- [ ] Cache invalidation handled +- [ ] Cache keys are unique and predictable +- [ ] TTL values appropriate + +### Scalability +- [ ] Horizontal scaling considered +- [ ] Bottlenecks identified +- [ ] Async processing for long operations +- [ ] Batch operations where appropriate + +--- + +## Maintainability + +### Code Quality +- [ ] Functions/methods have single responsibility +- [ ] Classes follow SOLID principles +- [ ] Code is DRY (Don't Repeat Yourself) +- [ ] No dead code or commented-out code +- [ ] Magic numbers replaced with constants + +### Naming +- [ ] Names are descriptive and consistent +- [ ] Naming follows project conventions +- [ ] No abbreviations that obscure meaning +- [ ] Boolean variables/functions have is/has/can prefix + +### Structure +- [ ] Functions are appropriately sized (<50 lines preferred) +- [ ] Nesting depth is reasonable (<4 levels) +- [ ] Related code is grouped together +- [ ] Dependencies are minimal and explicit + +### Readability +- [ ] Code is self-documenting where possible +- [ ] Complex logic has explanatory comments +- [ ] Formatting is consistent +- [ ] No overly clever or obscure code + +--- + +## Testing + +### Coverage +- [ ] New code has unit tests +- [ ] Critical paths have integration tests +- [ ] Edge cases are tested +- [ ] Error conditions are tested + +### Quality +- [ ] Tests are independent +- [ ] Tests have clear assertions +- [ ] Test names describe what is tested +- [ ] Tests don't depend on external state + +### Mocking +- [ ] External dependencies are mocked +- [ ] Mocks are realistic +- [ ] Mock setup is not excessive + +--- + +## Documentation + +### Code Documentation +- [ ] Public APIs are documented +- [ ] Complex algorithms explained +- [ ] Non-obvious decisions documented +- [ ] TODO/FIXME comments have context + +### External Documentation +- [ ] README updated if needed +- [ ] API documentation updated +- [ ] Changelog updated +- [ ] Migration guides provided + +--- + +## Language-Specific Checks + +### TypeScript/JavaScript +- [ ] Types are explicit (avoid `any`) +- [ ] Null checks present (`?.`, `??`) +- [ ] Async/await errors handled +- [ ] No floating promises +- [ ] Memory leaks from closures checked + +### Python +- [ ] Type hints used for public APIs +- [ ] Context managers for resources (`with` statements) +- [ ] Exception handling is specific (not bare `except`) +- [ ] No mutable default arguments +- [ ] List comprehensions used appropriately + +### Go +- [ ] Errors checked and handled +- [ ] Goroutine leaks prevented +- [ ] Context propagation correct +- [ ] Defer statements in right order +- [ ] Interfaces minimal + +### Swift +- [ ] Optionals handled safely +- [ ] Memory management correct (weak/unowned) +- [ ] Error handling uses Result or throws +- [ ] Access control appropriate +- [ ] Codable implementation correct + +### Kotlin +- [ ] Null safety leveraged +- [ ] Coroutine cancellation handled +- [ ] Data classes used appropriately +- [ ] Extension functions don't obscure behavior +- [ ] Sealed classes for state + +--- + +## Review Process Tips + +### Before Approving +1. Verify all critical checks passed +2. Confirm tests are adequate +3. Consider deployment impact +4. Check for any security concerns +5. Ensure documentation is updated + +### Providing Feedback +- Be specific about issues +- Explain why something is problematic +- Suggest alternatives when possible +- Distinguish blockers from suggestions +- Acknowledge good patterns + +### When to Block +- Security vulnerabilities present +- Critical logic errors +- No tests for risky changes +- Breaking changes without migration +- Significant performance regressions diff --git a/engineering-team/code-reviewer/references/coding_standards.md b/engineering-team/code-reviewer/references/coding_standards.md index b36bb6c..9fbc6a0 100644 --- a/engineering-team/code-reviewer/references/coding_standards.md +++ b/engineering-team/code-reviewer/references/coding_standards.md @@ -1,103 +1,555 @@ # Coding Standards -## Overview +Language-specific coding standards and conventions for code review. -This reference guide provides comprehensive information for code reviewer. +--- -## Patterns and Practices +## Table of Contents -### Pattern 1: Best Practice Implementation +- [Universal Principles](#universal-principles) +- [TypeScript Standards](#typescript-standards) +- [JavaScript Standards](#javascript-standards) +- [Python Standards](#python-standards) +- [Go Standards](#go-standards) +- [Swift Standards](#swift-standards) +- [Kotlin Standards](#kotlin-standards) -**Description:** -Detailed explanation of the pattern. +--- -**When to Use:** -- Scenario 1 -- Scenario 2 -- Scenario 3 +## Universal Principles + +These apply across all languages. + +### Naming Conventions + +| Element | Convention | Example | +|---------|------------|---------| +| Variables | camelCase (JS/TS), snake_case (Python/Go) | `userName`, `user_name` | +| Constants | SCREAMING_SNAKE_CASE | `MAX_RETRY_COUNT` | +| Functions | camelCase (JS/TS), snake_case (Python) | `getUserById`, `get_user_by_id` | +| Classes | PascalCase | `UserRepository` | +| Interfaces | PascalCase, optionally prefixed | `IUserService` or `UserService` | +| Private members | Prefix with underscore or use access modifiers | `_internalState` | + +### Function Design + +``` +Good functions: +- Do one thing well +- Have descriptive names (verb + noun) +- Take 3 or fewer parameters +- Return early for error cases +- Stay under 50 lines +``` + +### Error Handling + +``` +Good error handling: +- Catch specific errors, not generic exceptions +- Log with context (what, where, why) +- Clean up resources in error paths +- Don't swallow errors silently +- Provide actionable error messages +``` + +--- + +## TypeScript Standards + +### Type Annotations -**Implementation:** ```typescript -// Example code implementation -export class Example { - // Implementation details +// Avoid 'any' - use unknown for truly unknown types +function processData(data: unknown): ProcessedResult { + if (isValidData(data)) { + return transform(data); + } + throw new Error('Invalid data format'); +} + +// Use explicit return types for public APIs +export function calculateTotal(items: CartItem[]): number { + return items.reduce((sum, item) => sum + item.price, 0); +} + +// Use type guards for runtime checks +function isUser(obj: unknown): obj is User { + return ( + typeof obj === 'object' && + obj !== null && + 'id' in obj && + 'email' in obj + ); } ``` -**Benefits:** -- Benefit 1 -- Benefit 2 -- Benefit 3 +### Null Safety -**Trade-offs:** -- Consider 1 -- Consider 2 -- Consider 3 - -### Pattern 2: Advanced Technique - -**Description:** -Another important pattern for code reviewer. - -**Implementation:** ```typescript -// Advanced example -async function advancedExample() { - // Code here +// Use optional chaining and nullish coalescing +const userName = user?.profile?.name ?? 'Anonymous'; + +// Be explicit about nullable types +interface Config { + timeout: number; + retries?: number; // Optional + fallbackUrl: string | null; // Explicitly nullable +} + +// Use assertion functions for validation +function assertDefined(value: T | null | undefined): asserts value is T { + if (value === null || value === undefined) { + throw new Error('Value is not defined'); + } } ``` -## Guidelines +### Async/Await -### Code Organization -- Clear structure -- Logical separation -- Consistent naming -- Proper documentation +```typescript +// Always handle errors in async functions +async function fetchUser(id: string): Promise { + try { + const response = await api.get(`/users/${id}`); + return response.data; + } catch (error) { + logger.error('Failed to fetch user', { id, error }); + throw new UserFetchError(id, error); + } +} -### Performance Considerations -- Optimization strategies -- Bottleneck identification -- Monitoring approaches -- Scaling techniques +// Use Promise.all for parallel operations +async function loadDashboard(userId: string): Promise { + const [profile, stats, notifications] = await Promise.all([ + fetchProfile(userId), + fetchStats(userId), + fetchNotifications(userId) + ]); + return { profile, stats, notifications }; +} +``` -### Security Best Practices -- Input validation -- Authentication -- Authorization -- Data protection +### React/Component Standards -## Common Patterns +```typescript +// Use explicit prop types +interface ButtonProps { + label: string; + onClick: () => void; + variant?: 'primary' | 'secondary'; + disabled?: boolean; +} -### Pattern A -Implementation details and examples. +// Prefer functional components with hooks +function Button({ label, onClick, variant = 'primary', disabled = false }: ButtonProps) { + return ( + + ); +} -### Pattern B -Implementation details and examples. +// Use custom hooks for reusable logic +function useDebounce(value: T, delay: number): T { + const [debouncedValue, setDebouncedValue] = useState(value); -### Pattern C -Implementation details and examples. + useEffect(() => { + const timer = setTimeout(() => setDebouncedValue(value), delay); + return () => clearTimeout(timer); + }, [value, delay]); -## Anti-Patterns to Avoid + return debouncedValue; +} +``` -### Anti-Pattern 1 -What not to do and why. +--- -### Anti-Pattern 2 -What not to do and why. +## JavaScript Standards -## Tools and Resources +### Variable Declarations -### Recommended Tools -- Tool 1: Purpose -- Tool 2: Purpose -- Tool 3: Purpose +```javascript +// Use const by default, let when reassignment needed +const MAX_ITEMS = 100; +let currentCount = 0; -### Further Reading -- Resource 1 -- Resource 2 -- Resource 3 +// Never use var +// var is function-scoped and hoisted, leading to bugs +``` -## Conclusion +### Object and Array Patterns -Key takeaways for using this reference guide effectively. +```javascript +// Use object destructuring +const { name, email, role = 'user' } = user; + +// Use spread for immutable updates +const updatedUser = { ...user, lastLogin: new Date() }; +const updatedList = [...items, newItem]; + +// Use array methods over loops +const activeUsers = users.filter(u => u.isActive); +const emails = users.map(u => u.email); +const total = orders.reduce((sum, o) => sum + o.amount, 0); +``` + +### Module Patterns + +```javascript +// Use named exports for utilities +export function formatDate(date) { ... } +export function parseDate(str) { ... } + +// Use default export for main component/class +export default class UserService { ... } + +// Group related exports +export { formatDate, parseDate, isValidDate } from './dateUtils'; +``` + +--- + +## Python Standards + +### Type Hints (PEP 484) + +```python +from typing import Optional, List, Dict, Union + +def get_user(user_id: int) -> Optional[User]: + """Fetch user by ID, returns None if not found.""" + return db.query(User).filter(User.id == user_id).first() + +def process_items(items: List[str]) -> Dict[str, int]: + """Count occurrences of each item.""" + return {item: items.count(item) for item in set(items)} + +def send_notification( + user: User, + message: str, + *, + priority: str = "normal", + channels: List[str] = None +) -> bool: + """Send notification to user via specified channels.""" + channels = channels or ["email"] + # Implementation +``` + +### Exception Handling + +```python +# Catch specific exceptions +try: + result = api_client.fetch_data(endpoint) +except ConnectionError as e: + logger.warning(f"Connection failed: {e}") + return cached_data +except TimeoutError as e: + logger.error(f"Request timed out: {e}") + raise ServiceUnavailableError() from e + +# Use context managers for resources +with open(filepath, 'r') as f: + data = json.load(f) + +# Custom exceptions should be informative +class ValidationError(Exception): + def __init__(self, field: str, message: str): + self.field = field + self.message = message + super().__init__(f"{field}: {message}") +``` + +### Class Design + +```python +from dataclasses import dataclass +from abc import ABC, abstractmethod + +# Use dataclasses for data containers +@dataclass +class UserDTO: + id: int + email: str + name: str + is_active: bool = True + +# Use ABC for interfaces +class Repository(ABC): + @abstractmethod + def find_by_id(self, id: int) -> Optional[Entity]: + pass + + @abstractmethod + def save(self, entity: Entity) -> Entity: + pass + +# Use properties for computed attributes +class Order: + def __init__(self, items: List[OrderItem]): + self._items = items + + @property + def total(self) -> Decimal: + return sum(item.price * item.quantity for item in self._items) +``` + +--- + +## Go Standards + +### Error Handling + +```go +// Always check errors +file, err := os.Open(filename) +if err != nil { + return fmt.Errorf("failed to open %s: %w", filename, err) +} +defer file.Close() + +// Use custom error types for specific cases +type ValidationError struct { + Field string + Message string +} + +func (e *ValidationError) Error() string { + return fmt.Sprintf("%s: %s", e.Field, e.Message) +} + +// Wrap errors with context +if err := db.Query(query); err != nil { + return fmt.Errorf("query failed for user %d: %w", userID, err) +} +``` + +### Struct Design + +```go +// Use unexported fields with exported methods +type UserService struct { + repo UserRepository + cache Cache + logger Logger +} + +// Constructor functions for initialization +func NewUserService(repo UserRepository, cache Cache, logger Logger) *UserService { + return &UserService{ + repo: repo, + cache: cache, + logger: logger, + } +} + +// Keep interfaces small +type Reader interface { + Read(p []byte) (n int, err error) +} + +type Writer interface { + Write(p []byte) (n int, err error) +} +``` + +### Concurrency + +```go +// Use context for cancellation +func fetchData(ctx context.Context, url string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, err + } + // ... +} + +// Use channels for communication +func worker(jobs <-chan Job, results chan<- Result) { + for job := range jobs { + result := process(job) + results <- result + } +} + +// Use sync.WaitGroup for coordination +var wg sync.WaitGroup +for _, item := range items { + wg.Add(1) + go func(i Item) { + defer wg.Done() + processItem(i) + }(item) +} +wg.Wait() +``` + +--- + +## Swift Standards + +### Optionals + +```swift +// Use optional binding +if let user = fetchUser(id: userId) { + displayProfile(user) +} + +// Use guard for early exit +guard let data = response.data else { + throw NetworkError.noData +} + +// Use nil coalescing for defaults +let displayName = user.nickname ?? user.email + +// Avoid force unwrapping except in tests +// BAD: let name = user.name! +// GOOD: guard let name = user.name else { return } +``` + +### Protocol-Oriented Design + +```swift +// Define protocols with minimal requirements +protocol Identifiable { + var id: String { get } +} + +protocol Persistable: Identifiable { + func save() throws + static func find(by id: String) -> Self? +} + +// Use protocol extensions for default implementations +extension Persistable { + func save() throws { + try Storage.shared.save(self) + } +} + +// Prefer composition over inheritance +struct User: Identifiable, Codable { + let id: String + var name: String + var email: String +} +``` + +### Error Handling + +```swift +// Define domain-specific errors +enum AuthError: Error { + case invalidCredentials + case tokenExpired + case networkFailure(underlying: Error) +} + +// Use Result type for async operations +func authenticate( + email: String, + password: String, + completion: @escaping (Result) -> Void +) + +// Use throws for synchronous operations +func validate(_ input: String) throws -> ValidatedInput { + guard !input.isEmpty else { + throw ValidationError.emptyInput + } + return ValidatedInput(value: input) +} +``` + +--- + +## Kotlin Standards + +### Null Safety + +```kotlin +// Use nullable types explicitly +fun findUser(id: Int): User? { + return userRepository.find(id) +} + +// Use safe calls and elvis operator +val name = user?.profile?.name ?: "Unknown" + +// Use let for null checks with side effects +user?.let { activeUser -> + sendWelcomeEmail(activeUser.email) + logActivity(activeUser.id) +} + +// Use require/check for validation +fun processPayment(amount: Double) { + require(amount > 0) { "Amount must be positive: $amount" } + // Process +} +``` + +### Data Classes and Sealed Classes + +```kotlin +// Use data classes for DTOs +data class UserDTO( + val id: Int, + val email: String, + val name: String, + val isActive: Boolean = true +) + +// Use sealed classes for state +sealed class Result { + data class Success(val data: T) : Result() + data class Error(val message: String, val cause: Throwable? = null) : Result() + object Loading : Result() +} + +// Pattern matching with when +fun handleResult(result: Result) = when (result) { + is Result.Success -> showUser(result.data) + is Result.Error -> showError(result.message) + Result.Loading -> showLoading() +} +``` + +### Coroutines + +```kotlin +// Use structured concurrency +suspend fun loadDashboard(): Dashboard = coroutineScope { + val profile = async { fetchProfile() } + val stats = async { fetchStats() } + val notifications = async { fetchNotifications() } + + Dashboard( + profile = profile.await(), + stats = stats.await(), + notifications = notifications.await() + ) +} + +// Handle cancellation +suspend fun fetchWithRetry(url: String): Response { + repeat(3) { attempt -> + try { + return httpClient.get(url) + } catch (e: IOException) { + if (attempt == 2) throw e + delay(1000L * (attempt + 1)) + } + } + throw IllegalStateException("Unreachable") +} +``` diff --git a/engineering-team/code-reviewer/references/common_antipatterns.md b/engineering-team/code-reviewer/references/common_antipatterns.md index 19a2ded..2604545 100644 --- a/engineering-team/code-reviewer/references/common_antipatterns.md +++ b/engineering-team/code-reviewer/references/common_antipatterns.md @@ -1,103 +1,739 @@ # Common Antipatterns -## Overview +Code antipatterns to identify during review, with examples and fixes. -This reference guide provides comprehensive information for code reviewer. +--- -## Patterns and Practices +## Table of Contents -### Pattern 1: Best Practice Implementation +- [Structural Antipatterns](#structural-antipatterns) +- [Logic Antipatterns](#logic-antipatterns) +- [Security Antipatterns](#security-antipatterns) +- [Performance Antipatterns](#performance-antipatterns) +- [Testing Antipatterns](#testing-antipatterns) +- [Async Antipatterns](#async-antipatterns) -**Description:** -Detailed explanation of the pattern. +--- -**When to Use:** -- Scenario 1 -- Scenario 2 -- Scenario 3 +## Structural Antipatterns + +### God Class + +A class that does too much and knows too much. -**Implementation:** ```typescript -// Example code implementation -export class Example { - // Implementation details +// BAD: God class handling everything +class UserManager { + createUser(data: UserData) { ... } + updateUser(id: string, data: UserData) { ... } + deleteUser(id: string) { ... } + sendEmail(userId: string, content: string) { ... } + generateReport(userId: string) { ... } + validatePassword(password: string) { ... } + hashPassword(password: string) { ... } + uploadAvatar(userId: string, file: File) { ... } + resizeImage(file: File) { ... } + logActivity(userId: string, action: string) { ... } + // 50 more methods... +} + +// GOOD: Single responsibility classes +class UserRepository { + create(data: UserData): User { ... } + update(id: string, data: Partial): User { ... } + delete(id: string): void { ... } +} + +class EmailService { + send(to: string, content: string): void { ... } +} + +class PasswordService { + validate(password: string): ValidationResult { ... } + hash(password: string): string { ... } } ``` -**Benefits:** -- Benefit 1 -- Benefit 2 -- Benefit 3 +**Detection:** Class has >20 methods, >500 lines, or handles unrelated concerns. -**Trade-offs:** -- Consider 1 -- Consider 2 -- Consider 3 +--- -### Pattern 2: Advanced Technique +### Long Method -**Description:** -Another important pattern for code reviewer. +Functions that do too much and are hard to understand. -**Implementation:** -```typescript -// Advanced example -async function advancedExample() { - // Code here +```python +# BAD: Long method doing everything +def process_order(order_data): + # Validate order (20 lines) + if not order_data.get('items'): + raise ValueError('No items') + if not order_data.get('customer_id'): + raise ValueError('No customer') + # ... more validation + + # Calculate totals (30 lines) + subtotal = 0 + for item in order_data['items']: + price = get_product_price(item['product_id']) + subtotal += price * item['quantity'] + # ... tax calculation, discounts + + # Process payment (40 lines) + payment_result = payment_gateway.charge(...) + # ... handle payment errors + + # Create order record (20 lines) + order = Order.create(...) + + # Send notifications (20 lines) + send_order_confirmation(...) + notify_warehouse(...) + + return order + +# GOOD: Composed of focused functions +def process_order(order_data): + validate_order(order_data) + totals = calculate_order_totals(order_data) + payment = process_payment(order_data['customer_id'], totals) + order = create_order_record(order_data, totals, payment) + send_order_notifications(order) + return order +``` + +**Detection:** Function >50 lines or requires scrolling to read. + +--- + +### Deep Nesting + +Excessive indentation making code hard to follow. + +```javascript +// BAD: Deep nesting +function processData(data) { + if (data) { + if (data.items) { + if (data.items.length > 0) { + for (const item of data.items) { + if (item.isValid) { + if (item.type === 'premium') { + if (item.price > 100) { + // Finally do something + processItem(item); + } + } + } + } + } + } + } +} + +// GOOD: Early returns and guard clauses +function processData(data) { + if (!data?.items?.length) { + return; + } + + const premiumItems = data.items.filter( + item => item.isValid && item.type === 'premium' && item.price > 100 + ); + + premiumItems.forEach(processItem); } ``` -## Guidelines +**Detection:** Indentation >4 levels deep. -### Code Organization -- Clear structure -- Logical separation -- Consistent naming -- Proper documentation +--- -### Performance Considerations -- Optimization strategies -- Bottleneck identification -- Monitoring approaches -- Scaling techniques +### Magic Numbers and Strings -### Security Best Practices -- Input validation -- Authentication -- Authorization -- Data protection +Hard-coded values without explanation. -## Common Patterns +```go +// BAD: Magic numbers +func calculateDiscount(total float64, userType int) float64 { + if userType == 1 { + return total * 0.15 + } else if userType == 2 { + return total * 0.25 + } + return total * 0.05 +} -### Pattern A -Implementation details and examples. +// GOOD: Named constants +const ( + UserTypeRegular = 1 + UserTypePremium = 2 -### Pattern B -Implementation details and examples. + DiscountRegular = 0.05 + DiscountStandard = 0.15 + DiscountPremium = 0.25 +) -### Pattern C -Implementation details and examples. +func calculateDiscount(total float64, userType int) float64 { + switch userType { + case UserTypePremium: + return total * DiscountPremium + case UserTypeRegular: + return total * DiscountStandard + default: + return total * DiscountRegular + } +} +``` -## Anti-Patterns to Avoid +**Detection:** Literal numbers (except 0, 1) or repeated string literals. -### Anti-Pattern 1 -What not to do and why. +--- -### Anti-Pattern 2 -What not to do and why. +### Primitive Obsession -## Tools and Resources +Using primitives instead of small objects. -### Recommended Tools -- Tool 1: Purpose -- Tool 2: Purpose -- Tool 3: Purpose +```typescript +// BAD: Primitives everywhere +function createUser( + name: string, + email: string, + phone: string, + street: string, + city: string, + zipCode: string, + country: string +): User { ... } -### Further Reading -- Resource 1 -- Resource 2 -- Resource 3 +// GOOD: Value objects +interface Address { + street: string; + city: string; + zipCode: string; + country: string; +} -## Conclusion +interface ContactInfo { + email: string; + phone: string; +} -Key takeaways for using this reference guide effectively. +function createUser( + name: string, + contact: ContactInfo, + address: Address +): User { ... } +``` + +**Detection:** Functions with >4 parameters of same type, or related primitives always passed together. + +--- + +## Logic Antipatterns + +### Boolean Blindness + +Passing booleans that make code unreadable at call sites. + +```swift +// BAD: What do these booleans mean? +user.configure(true, false, true, false) + +// GOOD: Named parameters or option objects +user.configure( + sendWelcomeEmail: true, + requireVerification: false, + enableNotifications: true, + isAdmin: false +) + +// Or use an options struct +struct UserConfiguration { + var sendWelcomeEmail: Bool = true + var requireVerification: Bool = false + var enableNotifications: Bool = true + var isAdmin: Bool = false +} + +user.configure(UserConfiguration()) +``` + +**Detection:** Function calls with multiple boolean literals. + +--- + +### Null Returns for Collections + +Returning null instead of empty collections. + +```kotlin +// BAD: Returning null +fun findUsersByRole(role: String): List? { + val users = repository.findByRole(role) + return if (users.isEmpty()) null else users +} + +// Caller must handle null +val users = findUsersByRole("admin") +if (users != null) { + users.forEach { ... } +} + +// GOOD: Return empty collection +fun findUsersByRole(role: String): List { + return repository.findByRole(role) +} + +// Caller can iterate directly +findUsersByRole("admin").forEach { ... } +``` + +**Detection:** Functions returning nullable collections. + +--- + +### Stringly Typed Code + +Using strings where enums or types should be used. + +```python +# BAD: String-based logic +def handle_event(event_type: str, data: dict): + if event_type == "user_created": + handle_user_created(data) + elif event_type == "user_updated": + handle_user_updated(data) + elif event_type == "user_dleted": # Typo won't be caught + handle_user_deleted(data) + +# GOOD: Enum-based +from enum import Enum + +class EventType(Enum): + USER_CREATED = "user_created" + USER_UPDATED = "user_updated" + USER_DELETED = "user_deleted" + +def handle_event(event_type: EventType, data: dict): + handlers = { + EventType.USER_CREATED: handle_user_created, + EventType.USER_UPDATED: handle_user_updated, + EventType.USER_DELETED: handle_user_deleted, + } + handlers[event_type](data) +``` + +**Detection:** String comparisons for type/status/category values. + +--- + +## Security Antipatterns + +### SQL Injection + +String concatenation in SQL queries. + +```javascript +// BAD: String concatenation +const query = `SELECT * FROM users WHERE id = ${userId}`; +db.query(query); + +// BAD: String templates still vulnerable +const query = `SELECT * FROM users WHERE name = '${userName}'`; + +// GOOD: Parameterized queries +const query = 'SELECT * FROM users WHERE id = $1'; +db.query(query, [userId]); + +// GOOD: Using ORM safely +User.findOne({ where: { id: userId } }); +``` + +**Detection:** String concatenation or template literals with SQL keywords. + +--- + +### Hardcoded Credentials + +Secrets in source code. + +```python +# BAD: Hardcoded secrets +API_KEY = "sk-abc123xyz789" +DATABASE_URL = "postgresql://admin:password123@prod-db.internal:5432/app" + +# GOOD: Environment variables +import os + +API_KEY = os.environ["API_KEY"] +DATABASE_URL = os.environ["DATABASE_URL"] + +# GOOD: Secrets manager +from aws_secretsmanager import get_secret + +API_KEY = get_secret("api-key") +``` + +**Detection:** Variables named `password`, `secret`, `key`, `token` with string literals. + +--- + +### Unsafe Deserialization + +Deserializing untrusted data without validation. + +```python +# BAD: Binary serialization from untrusted source can execute arbitrary code +# Examples: Python's binary serialization, yaml.load without SafeLoader + +# GOOD: Use safe alternatives +import json + +def load_data(file_path): + with open(file_path, 'r') as f: + return json.load(f) + +# GOOD: Use SafeLoader for YAML +import yaml + +with open('config.yaml') as f: + config = yaml.safe_load(f) +``` + +**Detection:** Binary deserialization functions, yaml.load without safe loader, dynamic code execution on external data. + +--- + +### Missing Input Validation + +Trusting user input without validation. + +```typescript +// BAD: No validation +app.post('/user', (req, res) => { + const user = db.create({ + name: req.body.name, + email: req.body.email, + role: req.body.role // User can set themselves as admin! + }); + res.json(user); +}); + +// GOOD: Validate and sanitize +import { z } from 'zod'; + +const CreateUserSchema = z.object({ + name: z.string().min(1).max(100), + email: z.string().email(), + // role is NOT accepted from input +}); + +app.post('/user', (req, res) => { + const validated = CreateUserSchema.parse(req.body); + const user = db.create({ + ...validated, + role: 'user' // Default role, not from input + }); + res.json(user); +}); +``` + +**Detection:** Request body/params used directly without validation schema. + +--- + +## Performance Antipatterns + +### N+1 Query Problem + +Loading related data one record at a time. + +```python +# BAD: N+1 queries +def get_orders_with_items(): + orders = Order.query.all() # 1 query + for order in orders: + items = OrderItem.query.filter_by(order_id=order.id).all() # N queries + order.items = items + return orders + +# GOOD: Eager loading +def get_orders_with_items(): + return Order.query.options( + joinedload(Order.items) + ).all() # 1 query with JOIN + +# GOOD: Batch loading +def get_orders_with_items(): + orders = Order.query.all() + order_ids = [o.id for o in orders] + items = OrderItem.query.filter( + OrderItem.order_id.in_(order_ids) + ).all() # 2 queries total + # Group items by order_id... +``` + +**Detection:** Database queries inside loops. + +--- + +### Unbounded Collections + +Loading unlimited data into memory. + +```go +// BAD: Load all records +func GetAllUsers() ([]User, error) { + return db.Find(&[]User{}) // Could be millions +} + +// GOOD: Pagination +func GetUsers(page, pageSize int) ([]User, error) { + offset := (page - 1) * pageSize + return db.Limit(pageSize).Offset(offset).Find(&[]User{}) +} + +// GOOD: Streaming for large datasets +func ProcessAllUsers(handler func(User) error) error { + rows, err := db.Model(&User{}).Rows() + if err != nil { + return err + } + defer rows.Close() + + for rows.Next() { + var user User + db.ScanRows(rows, &user) + if err := handler(user); err != nil { + return err + } + } + return nil +} +``` + +**Detection:** `findAll()`, `find({})`, or queries without `LIMIT`. + +--- + +### Synchronous I/O in Hot Paths + +Blocking operations in request handlers. + +```javascript +// BAD: Sync file read on every request +app.get('/config', (req, res) => { + const config = fs.readFileSync('./config.json'); // Blocks event loop + res.json(JSON.parse(config)); +}); + +// GOOD: Load once at startup +const config = JSON.parse(fs.readFileSync('./config.json')); + +app.get('/config', (req, res) => { + res.json(config); +}); + +// GOOD: Async with caching +let configCache = null; + +app.get('/config', async (req, res) => { + if (!configCache) { + configCache = JSON.parse(await fs.promises.readFile('./config.json')); + } + res.json(configCache); +}); +``` + +**Detection:** `readFileSync`, `execSync`, or blocking calls in request handlers. + +--- + +## Testing Antipatterns + +### Test Code Duplication + +Repeating setup in every test. + +```typescript +// BAD: Duplicate setup +describe('UserService', () => { + it('should create user', async () => { + const db = await createTestDatabase(); + const userRepo = new UserRepository(db); + const emailService = new MockEmailService(); + const service = new UserService(userRepo, emailService); + + const user = await service.create({ name: 'Test' }); + expect(user.name).toBe('Test'); + }); + + it('should update user', async () => { + const db = await createTestDatabase(); // Duplicated + const userRepo = new UserRepository(db); // Duplicated + const emailService = new MockEmailService(); // Duplicated + const service = new UserService(userRepo, emailService); // Duplicated + + // ... + }); +}); + +// GOOD: Shared setup +describe('UserService', () => { + let service: UserService; + let db: TestDatabase; + + beforeEach(async () => { + db = await createTestDatabase(); + const userRepo = new UserRepository(db); + const emailService = new MockEmailService(); + service = new UserService(userRepo, emailService); + }); + + afterEach(async () => { + await db.cleanup(); + }); + + it('should create user', async () => { + const user = await service.create({ name: 'Test' }); + expect(user.name).toBe('Test'); + }); +}); +``` + +--- + +### Testing Implementation Instead of Behavior + +Tests coupled to internal implementation. + +```python +# BAD: Testing implementation details +def test_add_item_to_cart(): + cart = ShoppingCart() + cart.add_item(Product("Apple", 1.00)) + + # Testing internal structure + assert cart._items[0].name == "Apple" + assert cart._total == 1.00 + +# GOOD: Testing behavior +def test_add_item_to_cart(): + cart = ShoppingCart() + cart.add_item(Product("Apple", 1.00)) + + # Testing public behavior + assert cart.item_count == 1 + assert cart.total == 1.00 + assert cart.contains("Apple") +``` + +--- + +## Async Antipatterns + +### Floating Promises + +Promises without await or catch. + +```typescript +// BAD: Floating promise +async function saveUser(user: User) { + db.save(user); // Not awaited, errors lost + logger.info('User saved'); // Logs before save completes +} + +// BAD: Fire and forget in loop +for (const item of items) { + processItem(item); // All run in parallel, no error handling +} + +// GOOD: Await the promise +async function saveUser(user: User) { + await db.save(user); + logger.info('User saved'); +} + +// GOOD: Process with proper handling +await Promise.all(items.map(item => processItem(item))); + +// Or sequentially +for (const item of items) { + await processItem(item); +} +``` + +**Detection:** Async function calls without `await` or `.then()`. + +--- + +### Callback Hell + +Deeply nested callbacks. + +```javascript +// BAD: Callback hell +getUser(userId, (err, user) => { + if (err) return handleError(err); + getOrders(user.id, (err, orders) => { + if (err) return handleError(err); + getProducts(orders[0].productIds, (err, products) => { + if (err) return handleError(err); + renderPage(user, orders, products, (err) => { + if (err) return handleError(err); + console.log('Done'); + }); + }); + }); +}); + +// GOOD: Async/await +async function loadPage(userId) { + try { + const user = await getUser(userId); + const orders = await getOrders(user.id); + const products = await getProducts(orders[0].productIds); + await renderPage(user, orders, products); + console.log('Done'); + } catch (err) { + handleError(err); + } +} +``` + +**Detection:** >2 levels of callback nesting. + +--- + +### Async in Constructor + +Async operations in constructors. + +```typescript +// BAD: Async in constructor +class DatabaseConnection { + constructor(url: string) { + this.connect(url); // Fire-and-forget async + } + + private async connect(url: string) { + this.client = await createClient(url); + } +} + +// GOOD: Factory method +class DatabaseConnection { + private constructor(private client: Client) {} + + static async create(url: string): Promise { + const client = await createClient(url); + return new DatabaseConnection(client); + } +} + +// Usage +const db = await DatabaseConnection.create(url); +``` + +**Detection:** `async` calls or `.then()` in constructor. diff --git a/engineering-team/code-reviewer/scripts/code_quality_checker.py b/engineering-team/code-reviewer/scripts/code_quality_checker.py index 35d4196..128dc9d 100755 --- a/engineering-team/code-reviewer/scripts/code_quality_checker.py +++ b/engineering-team/code-reviewer/scripts/code_quality_checker.py @@ -1,114 +1,560 @@ #!/usr/bin/env python3 """ Code Quality Checker -Automated tool for code reviewer tasks + +Analyzes source code for quality issues, code smells, complexity metrics, +and SOLID principle violations. + +Usage: + python code_quality_checker.py /path/to/file.py + python code_quality_checker.py /path/to/directory --recursive + python code_quality_checker.py . --language typescript --json """ -import os -import sys -import json import argparse +import json +import re +import sys from pathlib import Path from typing import Dict, List, Optional -class CodeQualityChecker: - """Main class for code quality checker functionality""" - - def __init__(self, target_path: str, verbose: bool = False): - self.target_path = Path(target_path) - self.verbose = verbose - self.results = {} - - def run(self) -> Dict: - """Execute the main functionality""" - print(f"🚀 Running {self.__class__.__name__}...") - print(f"📁 Target: {self.target_path}") - - try: - self.validate_target() - self.analyze() - self.generate_report() - - print("✅ Completed successfully!") - return self.results - - except Exception as e: - print(f"❌ Error: {e}") - sys.exit(1) - - def validate_target(self): - """Validate the target path exists and is accessible""" - if not self.target_path.exists(): - raise ValueError(f"Target path does not exist: {self.target_path}") - - if self.verbose: - print(f"✓ Target validated: {self.target_path}") - - def analyze(self): - """Perform the main analysis or operation""" - if self.verbose: - print("📊 Analyzing...") - - # Main logic here - self.results['status'] = 'success' - self.results['target'] = str(self.target_path) - self.results['findings'] = [] - - # Add analysis results - if self.verbose: - print(f"✓ Analysis complete: {len(self.results.get('findings', []))} findings") - - def generate_report(self): - """Generate and display the report""" - print("\n" + "="*50) - print("REPORT") - print("="*50) - print(f"Target: {self.results.get('target')}") - print(f"Status: {self.results.get('status')}") - print(f"Findings: {len(self.results.get('findings', []))}") - print("="*50 + "\n") + +# Language-specific file extensions +LANGUAGE_EXTENSIONS = { + "python": [".py"], + "typescript": [".ts", ".tsx"], + "javascript": [".js", ".jsx", ".mjs"], + "go": [".go"], + "swift": [".swift"], + "kotlin": [".kt", ".kts"] +} + +# Code smell thresholds +THRESHOLDS = { + "long_function_lines": 50, + "too_many_parameters": 5, + "high_complexity": 10, + "god_class_methods": 20, + "max_imports": 15 +} + + +def get_file_extension(filepath: Path) -> str: + """Get file extension.""" + return filepath.suffix.lower() + + +def detect_language(filepath: Path) -> Optional[str]: + """Detect programming language from file extension.""" + ext = get_file_extension(filepath) + for lang, extensions in LANGUAGE_EXTENSIONS.items(): + if ext in extensions: + return lang + return None + + +def read_file_content(filepath: Path) -> str: + """Read file content safely.""" + try: + with open(filepath, "r", encoding="utf-8", errors="ignore") as f: + return f.read() + except Exception: + return "" + + +def calculate_cyclomatic_complexity(content: str) -> int: + """ + Estimate cyclomatic complexity based on control flow keywords. + """ + complexity = 1 # Base complexity + + # Control flow patterns that increase complexity + patterns = [ + r"\bif\b", + r"\belif\b", + r"\belse\b", + r"\bfor\b", + r"\bwhile\b", + r"\bcase\b", + r"\bcatch\b", + r"\bexcept\b", + r"\band\b", + r"\bor\b", + r"\|\|", + r"&&" + ] + + for pattern in patterns: + matches = re.findall(pattern, content, re.IGNORECASE) + complexity += len(matches) + + return complexity + + +def count_lines(content: str) -> Dict[str, int]: + """Count different types of lines in code.""" + lines = content.split("\n") + total = len(lines) + blank = sum(1 for line in lines if not line.strip()) + comment = 0 + + for line in lines: + stripped = line.strip() + if stripped.startswith("#") or stripped.startswith("//"): + comment += 1 + elif stripped.startswith("/*") or stripped.startswith("'''") or stripped.startswith('"""'): + comment += 1 + + code = total - blank - comment + + return { + "total": total, + "code": code, + "blank": blank, + "comment": comment + } + + +def find_functions(content: str, language: str) -> List[Dict]: + """Find function definitions and their metrics.""" + functions = [] + + # Language-specific function patterns + patterns = { + "python": r"def\s+(\w+)\s*\(([^)]*)\)", + "typescript": r"(?:function\s+(\w+)|(?:const|let|var)\s+(\w+)\s*=\s*(?:async\s+)?\([^)]*\)\s*=>)", + "javascript": r"(?:function\s+(\w+)|(?:const|let|var)\s+(\w+)\s*=\s*(?:async\s+)?\([^)]*\)\s*=>)", + "go": r"func\s+(?:\([^)]+\)\s+)?(\w+)\s*\(([^)]*)\)", + "swift": r"func\s+(\w+)\s*\(([^)]*)\)", + "kotlin": r"fun\s+(\w+)\s*\(([^)]*)\)" + } + + pattern = patterns.get(language, patterns["python"]) + matches = re.finditer(pattern, content, re.MULTILINE) + + for match in matches: + name = next((g for g in match.groups() if g), "anonymous") + params_str = match.group(2) if len(match.groups()) > 1 and match.group(2) else "" + + # Count parameters + params = [p.strip() for p in params_str.split(",") if p.strip()] + param_count = len(params) + + # Estimate function length + start_pos = match.end() + remaining = content[start_pos:] + + next_func = re.search(pattern, remaining) + if next_func: + func_body = remaining[:next_func.start()] + else: + func_body = remaining[:min(2000, len(remaining))] + + line_count = len(func_body.split("\n")) + complexity = calculate_cyclomatic_complexity(func_body) + + functions.append({ + "name": name, + "parameters": param_count, + "lines": line_count, + "complexity": complexity + }) + + return functions + + +def find_classes(content: str, language: str) -> List[Dict]: + """Find class definitions and their metrics.""" + classes = [] + + patterns = { + "python": r"class\s+(\w+)", + "typescript": r"class\s+(\w+)", + "javascript": r"class\s+(\w+)", + "go": r"type\s+(\w+)\s+struct", + "swift": r"class\s+(\w+)", + "kotlin": r"class\s+(\w+)" + } + + pattern = patterns.get(language, patterns["python"]) + matches = re.finditer(pattern, content) + + for match in matches: + name = match.group(1) + + start_pos = match.end() + remaining = content[start_pos:] + + next_class = re.search(pattern, remaining) + if next_class: + class_body = remaining[:next_class.start()] + else: + class_body = remaining + + # Count methods + method_patterns = { + "python": r"def\s+\w+\s*\(", + "typescript": r"(?:public|private|protected)?\s*\w+\s*\([^)]*\)\s*[:{]", + "javascript": r"\w+\s*\([^)]*\)\s*\{", + "go": r"func\s+\(", + "swift": r"func\s+\w+", + "kotlin": r"fun\s+\w+" + } + method_pattern = method_patterns.get(language, method_patterns["python"]) + methods = len(re.findall(method_pattern, class_body)) + + classes.append({ + "name": name, + "methods": methods, + "lines": len(class_body.split("\n")) + }) + + return classes + + +def check_code_smells(content: str, functions: List[Dict], classes: List[Dict]) -> List[Dict]: + """Check for code smells in the content.""" + smells = [] + + # Long functions + for func in functions: + if func["lines"] > THRESHOLDS["long_function_lines"]: + smells.append({ + "type": "long_function", + "severity": "medium", + "message": f"Function '{func['name']}' has {func['lines']} lines (max: {THRESHOLDS['long_function_lines']})", + "location": func["name"] + }) + + # Too many parameters + for func in functions: + if func["parameters"] > THRESHOLDS["too_many_parameters"]: + smells.append({ + "type": "too_many_parameters", + "severity": "low", + "message": f"Function '{func['name']}' has {func['parameters']} parameters (max: {THRESHOLDS['too_many_parameters']})", + "location": func["name"] + }) + + # High complexity + for func in functions: + if func["complexity"] > THRESHOLDS["high_complexity"]: + severity = "high" if func["complexity"] > 20 else "medium" + smells.append({ + "type": "high_complexity", + "severity": severity, + "message": f"Function '{func['name']}' has complexity {func['complexity']} (max: {THRESHOLDS['high_complexity']})", + "location": func["name"] + }) + + # God classes + for cls in classes: + if cls["methods"] > THRESHOLDS["god_class_methods"]: + smells.append({ + "type": "god_class", + "severity": "high", + "message": f"Class '{cls['name']}' has {cls['methods']} methods (max: {THRESHOLDS['god_class_methods']})", + "location": cls["name"] + }) + + # Magic numbers + magic_pattern = r"\b(? List[Dict]: + """Check for potential SOLID principle violations.""" + violations = [] + + # OCP: Type checking instead of polymorphism + type_checks = len(re.findall(r"isinstance\(|type\(.*\)\s*==|typeof\s+\w+\s*===", content)) + if type_checks > 2: + violations.append({ + "principle": "OCP", + "name": "Open/Closed Principle", + "severity": "medium", + "message": f"Found {type_checks} type checks - consider using polymorphism" + }) + + # LSP/ISP: NotImplementedError + not_impl = len(re.findall(r"raise\s+NotImplementedError|not\s+implemented", content, re.IGNORECASE)) + if not_impl: + violations.append({ + "principle": "LSP/ISP", + "name": "Liskov/Interface Segregation", + "severity": "low", + "message": f"Found {not_impl} unimplemented methods - may indicate oversized interface" + }) + + # DIP: Too many direct imports + imports = len(re.findall(r"^(?:import|from)\s+", content, re.MULTILINE)) + if imports > THRESHOLDS["max_imports"]: + violations.append({ + "principle": "DIP", + "name": "Dependency Inversion Principle", + "severity": "low", + "message": f"File has {imports} imports - consider dependency injection" + }) + + return violations + + +def calculate_quality_score( + line_metrics: Dict, + functions: List[Dict], + classes: List[Dict], + smells: List[Dict], + violations: List[Dict] +) -> int: + """Calculate overall quality score (0-100).""" + score = 100 + + # Deduct for code smells + for smell in smells: + if smell["severity"] == "high": + score -= 10 + elif smell["severity"] == "medium": + score -= 5 + elif smell["severity"] == "low": + score -= 2 + + # Deduct for SOLID violations + for violation in violations: + if violation["severity"] == "high": + score -= 8 + elif violation["severity"] == "medium": + score -= 4 + elif violation["severity"] == "low": + score -= 2 + + # Bonus for good comment ratio (10-30%) + if line_metrics["total"] > 0: + comment_ratio = line_metrics["comment"] / line_metrics["total"] + if 0.1 <= comment_ratio <= 0.3: + score += 5 + + # Bonus for reasonable function sizes + if functions: + avg_lines = sum(f["lines"] for f in functions) / len(functions) + if avg_lines < 30: + score += 5 + + return max(0, min(100, score)) + + +def get_grade(score: int) -> str: + """Convert score to letter grade.""" + if score >= 90: + return "A" + elif score >= 80: + return "B" + elif score >= 70: + return "C" + elif score >= 60: + return "D" + else: + return "F" + + +def analyze_file(filepath: Path) -> Dict: + """Analyze a single file for code quality.""" + language = detect_language(filepath) + if not language: + return {"error": f"Unsupported file type: {filepath.suffix}"} + + content = read_file_content(filepath) + if not content: + return {"error": f"Could not read file: {filepath}"} + + line_metrics = count_lines(content) + functions = find_functions(content, language) + classes = find_classes(content, language) + smells = check_code_smells(content, functions, classes) + violations = check_solid_violations(content) + score = calculate_quality_score(line_metrics, functions, classes, smells, violations) + + return { + "file": str(filepath), + "language": language, + "metrics": { + "lines": line_metrics, + "functions": len(functions), + "classes": len(classes), + "avg_complexity": round(sum(f["complexity"] for f in functions) / max(1, len(functions)), 1) + }, + "quality_score": score, + "grade": get_grade(score), + "smells": smells, + "solid_violations": violations, + "function_details": functions[:10], + "class_details": classes[:10] + } + + +def analyze_directory( + dir_path: Path, + recursive: bool = True, + language: Optional[str] = None +) -> Dict: + """Analyze all files in a directory.""" + results = [] + extensions = [] + + if language: + extensions = LANGUAGE_EXTENSIONS.get(language, []) + else: + for exts in LANGUAGE_EXTENSIONS.values(): + extensions.extend(exts) + + pattern = "**/*" if recursive else "*" + + for ext in extensions: + for filepath in dir_path.glob(f"{pattern}{ext}"): + if "node_modules" in str(filepath) or ".git" in str(filepath): + continue + result = analyze_file(filepath) + if "error" not in result: + results.append(result) + + if not results: + return {"error": "No supported files found"} + + total_score = sum(r["quality_score"] for r in results) + avg_score = total_score / len(results) + total_smells = sum(len(r["smells"]) for r in results) + total_violations = sum(len(r["solid_violations"]) for r in results) + + return { + "directory": str(dir_path), + "files_analyzed": len(results), + "average_score": round(avg_score, 1), + "overall_grade": get_grade(int(avg_score)), + "total_code_smells": total_smells, + "total_solid_violations": total_violations, + "files": sorted(results, key=lambda x: x["quality_score"]) + } + + +def print_report(analysis: Dict) -> None: + """Print human-readable analysis report.""" + if "error" in analysis: + print(f"Error: {analysis['error']}") + return + + print("=" * 60) + print("CODE QUALITY REPORT") + print("=" * 60) + + if "file" in analysis: + print(f"\nFile: {analysis['file']}") + print(f"Language: {analysis['language']}") + print(f"Quality Score: {analysis['quality_score']}/100 ({analysis['grade']})") + + metrics = analysis["metrics"] + print(f"\nLines: {metrics['lines']['total']} ({metrics['lines']['code']} code, {metrics['lines']['comment']} comments)") + print(f"Functions: {metrics['functions']}") + print(f"Classes: {metrics['classes']}") + print(f"Avg Complexity: {metrics['avg_complexity']}") + + if analysis["smells"]: + print("\n--- CODE SMELLS ---") + for smell in analysis["smells"][:10]: + print(f" [{smell['severity'].upper()}] {smell['message']} ({smell['location']})") + + if analysis["solid_violations"]: + print("\n--- SOLID VIOLATIONS ---") + for v in analysis["solid_violations"]: + print(f" [{v['principle']}] {v['message']}") + else: + print(f"\nDirectory: {analysis['directory']}") + print(f"Files Analyzed: {analysis['files_analyzed']}") + print(f"Average Score: {analysis['average_score']}/100 ({analysis['overall_grade']})") + print(f"Total Code Smells: {analysis['total_code_smells']}") + print(f"Total SOLID Violations: {analysis['total_solid_violations']}") + + print("\n--- FILES BY QUALITY ---") + for f in analysis["files"][:10]: + print(f" {f['quality_score']:3d}/100 [{f['grade']}] {f['file']}") + + print("\n" + "=" * 60) + def main(): - """Main entry point""" parser = argparse.ArgumentParser( - description="Code Quality Checker" + description="Analyze code quality, smells, and SOLID violations" ) parser.add_argument( - 'target', - help='Target path to analyze or process' + "path", + help="File or directory to analyze" ) parser.add_argument( - '--verbose', '-v', - action='store_true', - help='Enable verbose output' + "--recursive", "-r", + action="store_true", + default=True, + help="Recursively analyze directories (default: true)" ) parser.add_argument( - '--json', - action='store_true', - help='Output results as JSON' + "--language", "-l", + choices=list(LANGUAGE_EXTENSIONS.keys()), + help="Filter by programming language" ) parser.add_argument( - '--output', '-o', - help='Output file path' + "--json", + action="store_true", + help="Output in JSON format" ) - + parser.add_argument( + "--output", "-o", + help="Write output to file" + ) + args = parser.parse_args() - - tool = CodeQualityChecker( - args.target, - verbose=args.verbose - ) - - results = tool.run() - + + target = Path(args.path).resolve() + + if not target.exists(): + print(f"Error: Path does not exist: {target}", file=sys.stderr) + sys.exit(1) + + if target.is_file(): + analysis = analyze_file(target) + else: + analysis = analyze_directory(target, args.recursive, args.language) + if args.json: - output = json.dumps(results, indent=2) + output = json.dumps(analysis, indent=2, default=str) if args.output: - with open(args.output, 'w') as f: + with open(args.output, "w") as f: f.write(output) print(f"Results written to {args.output}") else: print(output) + else: + print_report(analysis) -if __name__ == '__main__': + +if __name__ == "__main__": main() diff --git a/engineering-team/code-reviewer/scripts/pr_analyzer.py b/engineering-team/code-reviewer/scripts/pr_analyzer.py index 926c06a..4cfd1b5 100755 --- a/engineering-team/code-reviewer/scripts/pr_analyzer.py +++ b/engineering-team/code-reviewer/scripts/pr_analyzer.py @@ -1,114 +1,495 @@ #!/usr/bin/env python3 """ -Pr Analyzer -Automated tool for code reviewer tasks +PR Analyzer + +Analyzes pull request changes for review complexity, risk assessment, +and generates review priorities. + +Usage: + python pr_analyzer.py /path/to/repo + python pr_analyzer.py . --base main --head feature-branch + python pr_analyzer.py /path/to/repo --json """ -import os -import sys -import json import argparse +import json +import os +import re +import subprocess +import sys from pathlib import Path -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple + + +# File categories for review prioritization +FILE_CATEGORIES = { + "critical": { + "patterns": [ + r"auth", r"security", r"password", r"token", r"secret", + r"payment", r"billing", r"crypto", r"encrypt" + ], + "weight": 5, + "description": "Security-sensitive files requiring careful review" + }, + "high": { + "patterns": [ + r"api", r"database", r"migration", r"schema", r"model", + r"config", r"env", r"middleware" + ], + "weight": 4, + "description": "Core infrastructure files" + }, + "medium": { + "patterns": [ + r"service", r"controller", r"handler", r"util", r"helper" + ], + "weight": 3, + "description": "Business logic files" + }, + "low": { + "patterns": [ + r"test", r"spec", r"mock", r"fixture", r"story", + r"readme", r"docs", r"\.md$" + ], + "weight": 1, + "description": "Tests and documentation" + } +} + +# Risky patterns to flag +RISK_PATTERNS = [ + { + "name": "hardcoded_secrets", + "pattern": r"(password|secret|api_key|token)\s*[=:]\s*['\"][^'\"]+['\"]", + "severity": "critical", + "message": "Potential hardcoded secret detected" + }, + { + "name": "todo_fixme", + "pattern": r"(TODO|FIXME|HACK|XXX):", + "severity": "low", + "message": "TODO/FIXME comment found" + }, + { + "name": "console_log", + "pattern": r"console\.(log|debug|info|warn|error)\(", + "severity": "medium", + "message": "Console statement found (remove for production)" + }, + { + "name": "debugger", + "pattern": r"\bdebugger\b", + "severity": "high", + "message": "Debugger statement found" + }, + { + "name": "disable_eslint", + "pattern": r"eslint-disable", + "severity": "medium", + "message": "ESLint rule disabled" + }, + { + "name": "any_type", + "pattern": r":\s*any\b", + "severity": "medium", + "message": "TypeScript 'any' type used" + }, + { + "name": "sql_concatenation", + "pattern": r"(SELECT|INSERT|UPDATE|DELETE).*\+.*['\"]", + "severity": "critical", + "message": "Potential SQL injection (string concatenation in query)" + } +] + + +def run_git_command(cmd: List[str], cwd: Path) -> Tuple[bool, str]: + """Run a git command and return success status and output.""" + try: + result = subprocess.run( + cmd, + cwd=cwd, + capture_output=True, + text=True, + timeout=30 + ) + return result.returncode == 0, result.stdout.strip() + except subprocess.TimeoutExpired: + return False, "Command timed out" + except Exception as e: + return False, str(e) + + +def get_changed_files(repo_path: Path, base: str, head: str) -> List[Dict]: + """Get list of changed files between two refs.""" + success, output = run_git_command( + ["git", "diff", "--name-status", f"{base}...{head}"], + repo_path + ) + + if not success: + # Try without the triple dot (for uncommitted changes) + success, output = run_git_command( + ["git", "diff", "--name-status", base, head], + repo_path + ) + + if not success or not output: + # Fall back to staged changes + success, output = run_git_command( + ["git", "diff", "--name-status", "--cached"], + repo_path + ) + + files = [] + for line in output.split("\n"): + if not line.strip(): + continue + parts = line.split("\t") + if len(parts) >= 2: + status = parts[0][0] # First character of status + filepath = parts[-1] # Handle renames (R100\told\tnew) + status_map = { + "A": "added", + "M": "modified", + "D": "deleted", + "R": "renamed", + "C": "copied" + } + files.append({ + "path": filepath, + "status": status_map.get(status, "modified") + }) + + return files + + +def get_file_diff(repo_path: Path, filepath: str, base: str, head: str) -> str: + """Get diff content for a specific file.""" + success, output = run_git_command( + ["git", "diff", f"{base}...{head}", "--", filepath], + repo_path + ) + if not success: + success, output = run_git_command( + ["git", "diff", "--cached", "--", filepath], + repo_path + ) + return output if success else "" + + +def categorize_file(filepath: str) -> Tuple[str, int]: + """Categorize a file based on its path and name.""" + filepath_lower = filepath.lower() + + for category, info in FILE_CATEGORIES.items(): + for pattern in info["patterns"]: + if re.search(pattern, filepath_lower): + return category, info["weight"] + + return "medium", 2 # Default category + + +def analyze_diff_for_risks(diff_content: str, filepath: str) -> List[Dict]: + """Analyze diff content for risky patterns.""" + risks = [] + + # Only analyze added lines (starting with +) + added_lines = [ + line[1:] for line in diff_content.split("\n") + if line.startswith("+") and not line.startswith("+++") + ] + + content = "\n".join(added_lines) + + for risk in RISK_PATTERNS: + matches = re.findall(risk["pattern"], content, re.IGNORECASE) + if matches: + risks.append({ + "name": risk["name"], + "severity": risk["severity"], + "message": risk["message"], + "file": filepath, + "count": len(matches) + }) + + return risks + + +def count_changes(diff_content: str) -> Dict[str, int]: + """Count additions and deletions in diff.""" + additions = 0 + deletions = 0 + + for line in diff_content.split("\n"): + if line.startswith("+") and not line.startswith("+++"): + additions += 1 + elif line.startswith("-") and not line.startswith("---"): + deletions += 1 + + return {"additions": additions, "deletions": deletions} + + +def calculate_complexity_score(files: List[Dict], all_risks: List[Dict]) -> int: + """Calculate overall PR complexity score (1-10).""" + score = 0 + + # File count contribution (max 3 points) + file_count = len(files) + if file_count > 20: + score += 3 + elif file_count > 10: + score += 2 + elif file_count > 5: + score += 1 + + # Total changes contribution (max 3 points) + total_changes = sum(f.get("additions", 0) + f.get("deletions", 0) for f in files) + if total_changes > 500: + score += 3 + elif total_changes > 200: + score += 2 + elif total_changes > 50: + score += 1 + + # Risk severity contribution (max 4 points) + critical_risks = sum(1 for r in all_risks if r["severity"] == "critical") + high_risks = sum(1 for r in all_risks if r["severity"] == "high") + + score += min(2, critical_risks) + score += min(2, high_risks) + + return min(10, max(1, score)) + + +def analyze_commit_messages(repo_path: Path, base: str, head: str) -> Dict: + """Analyze commit messages in the PR.""" + success, output = run_git_command( + ["git", "log", "--oneline", f"{base}...{head}"], + repo_path + ) + + if not success or not output: + return {"commits": 0, "issues": []} + + commits = output.strip().split("\n") + issues = [] + + for commit in commits: + if len(commit) < 10: + continue + + # Check for conventional commit format + message = commit[8:] if len(commit) > 8 else commit # Skip hash + + if not re.match(r"^(feat|fix|docs|style|refactor|test|chore|perf|ci|build|revert)(\(.+\))?:", message): + issues.append({ + "commit": commit[:7], + "issue": "Does not follow conventional commit format" + }) + + if len(message) > 72: + issues.append({ + "commit": commit[:7], + "issue": "Commit message exceeds 72 characters" + }) + + return { + "commits": len(commits), + "issues": issues + } + + +def analyze_pr( + repo_path: Path, + base: str = "main", + head: str = "HEAD" +) -> Dict: + """Perform complete PR analysis.""" + # Get changed files + changed_files = get_changed_files(repo_path, base, head) + + if not changed_files: + return { + "status": "no_changes", + "message": "No changes detected between branches" + } + + # Analyze each file + all_risks = [] + file_analyses = [] + + for file_info in changed_files: + filepath = file_info["path"] + category, weight = categorize_file(filepath) + + # Get diff for the file + diff = get_file_diff(repo_path, filepath, base, head) + changes = count_changes(diff) + risks = analyze_diff_for_risks(diff, filepath) + + all_risks.extend(risks) + + file_analyses.append({ + "path": filepath, + "status": file_info["status"], + "category": category, + "priority_weight": weight, + "additions": changes["additions"], + "deletions": changes["deletions"], + "risks": risks + }) + + # Sort by priority (highest first) + file_analyses.sort(key=lambda x: (-x["priority_weight"], x["path"])) + + # Analyze commits + commit_analysis = analyze_commit_messages(repo_path, base, head) + + # Calculate metrics + complexity = calculate_complexity_score(file_analyses, all_risks) + + total_additions = sum(f["additions"] for f in file_analyses) + total_deletions = sum(f["deletions"] for f in file_analyses) + + return { + "status": "analyzed", + "summary": { + "files_changed": len(file_analyses), + "total_additions": total_additions, + "total_deletions": total_deletions, + "complexity_score": complexity, + "complexity_label": get_complexity_label(complexity), + "commits": commit_analysis["commits"] + }, + "risks": { + "critical": [r for r in all_risks if r["severity"] == "critical"], + "high": [r for r in all_risks if r["severity"] == "high"], + "medium": [r for r in all_risks if r["severity"] == "medium"], + "low": [r for r in all_risks if r["severity"] == "low"] + }, + "files": file_analyses, + "commit_issues": commit_analysis["issues"], + "review_order": [f["path"] for f in file_analyses[:10]] # Top 10 priority files + } + + +def get_complexity_label(score: int) -> str: + """Get human-readable complexity label.""" + if score <= 2: + return "Simple" + elif score <= 4: + return "Moderate" + elif score <= 6: + return "Complex" + elif score <= 8: + return "Very Complex" + else: + return "Critical" + + +def print_report(analysis: Dict) -> None: + """Print human-readable analysis report.""" + if analysis["status"] == "no_changes": + print("No changes detected.") + return + + summary = analysis["summary"] + risks = analysis["risks"] + + print("=" * 60) + print("PR ANALYSIS REPORT") + print("=" * 60) + + print(f"\nComplexity: {summary['complexity_score']}/10 ({summary['complexity_label']})") + print(f"Files Changed: {summary['files_changed']}") + print(f"Lines: +{summary['total_additions']} / -{summary['total_deletions']}") + print(f"Commits: {summary['commits']}") + + # Risk summary + print("\n--- RISK SUMMARY ---") + print(f"Critical: {len(risks['critical'])}") + print(f"High: {len(risks['high'])}") + print(f"Medium: {len(risks['medium'])}") + print(f"Low: {len(risks['low'])}") + + # Critical and high risks details + if risks["critical"]: + print("\n--- CRITICAL RISKS ---") + for risk in risks["critical"]: + print(f" [{risk['file']}] {risk['message']} (x{risk['count']})") + + if risks["high"]: + print("\n--- HIGH RISKS ---") + for risk in risks["high"]: + print(f" [{risk['file']}] {risk['message']} (x{risk['count']})") + + # Commit message issues + if analysis["commit_issues"]: + print("\n--- COMMIT MESSAGE ISSUES ---") + for issue in analysis["commit_issues"][:5]: + print(f" {issue['commit']}: {issue['issue']}") + + # Review order + print("\n--- SUGGESTED REVIEW ORDER ---") + for i, filepath in enumerate(analysis["review_order"], 1): + file_info = next(f for f in analysis["files"] if f["path"] == filepath) + print(f" {i}. [{file_info['category'].upper()}] {filepath}") + + print("\n" + "=" * 60) -class PrAnalyzer: - """Main class for pr analyzer functionality""" - - def __init__(self, target_path: str, verbose: bool = False): - self.target_path = Path(target_path) - self.verbose = verbose - self.results = {} - - def run(self) -> Dict: - """Execute the main functionality""" - print(f"🚀 Running {self.__class__.__name__}...") - print(f"📁 Target: {self.target_path}") - - try: - self.validate_target() - self.analyze() - self.generate_report() - - print("✅ Completed successfully!") - return self.results - - except Exception as e: - print(f"❌ Error: {e}") - sys.exit(1) - - def validate_target(self): - """Validate the target path exists and is accessible""" - if not self.target_path.exists(): - raise ValueError(f"Target path does not exist: {self.target_path}") - - if self.verbose: - print(f"✓ Target validated: {self.target_path}") - - def analyze(self): - """Perform the main analysis or operation""" - if self.verbose: - print("📊 Analyzing...") - - # Main logic here - self.results['status'] = 'success' - self.results['target'] = str(self.target_path) - self.results['findings'] = [] - - # Add analysis results - if self.verbose: - print(f"✓ Analysis complete: {len(self.results.get('findings', []))} findings") - - def generate_report(self): - """Generate and display the report""" - print("\n" + "="*50) - print("REPORT") - print("="*50) - print(f"Target: {self.results.get('target')}") - print(f"Status: {self.results.get('status')}") - print(f"Findings: {len(self.results.get('findings', []))}") - print("="*50 + "\n") def main(): - """Main entry point""" parser = argparse.ArgumentParser( - description="Pr Analyzer" + description="Analyze pull request for review complexity and risks" ) parser.add_argument( - 'target', - help='Target path to analyze or process' + "repo_path", + nargs="?", + default=".", + help="Path to git repository (default: current directory)" ) parser.add_argument( - '--verbose', '-v', - action='store_true', - help='Enable verbose output' + "--base", "-b", + default="main", + help="Base branch for comparison (default: main)" ) parser.add_argument( - '--json', - action='store_true', - help='Output results as JSON' + "--head", "-h", + default="HEAD", + help="Head branch/commit for comparison (default: HEAD)" ) parser.add_argument( - '--output', '-o', - help='Output file path' + "--json", + action="store_true", + help="Output in JSON format" ) - + parser.add_argument( + "--output", "-o", + help="Write output to file" + ) + args = parser.parse_args() - - tool = PrAnalyzer( - args.target, - verbose=args.verbose - ) - - results = tool.run() - + + repo_path = Path(args.repo_path).resolve() + + if not (repo_path / ".git").exists(): + print(f"Error: {repo_path} is not a git repository", file=sys.stderr) + sys.exit(1) + + analysis = analyze_pr(repo_path, args.base, args.head) + if args.json: - output = json.dumps(results, indent=2) + output = json.dumps(analysis, indent=2) if args.output: - with open(args.output, 'w') as f: + with open(args.output, "w") as f: f.write(output) print(f"Results written to {args.output}") else: print(output) + else: + print_report(analysis) -if __name__ == '__main__': + +if __name__ == "__main__": main() diff --git a/engineering-team/code-reviewer/scripts/review_report_generator.py b/engineering-team/code-reviewer/scripts/review_report_generator.py index 0805302..7c2246a 100755 --- a/engineering-team/code-reviewer/scripts/review_report_generator.py +++ b/engineering-team/code-reviewer/scripts/review_report_generator.py @@ -1,114 +1,505 @@ #!/usr/bin/env python3 """ Review Report Generator -Automated tool for code reviewer tasks + +Generates comprehensive code review reports by combining PR analysis +and code quality findings into structured, actionable reports. + +Usage: + python review_report_generator.py /path/to/repo + python review_report_generator.py . --pr-analysis pr_results.json --quality-analysis quality_results.json + python review_report_generator.py /path/to/repo --format markdown --output review.md """ -import os -import sys -import json import argparse +import json +import os +import subprocess +import sys +from datetime import datetime from pathlib import Path -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple + + +# Severity weights for prioritization +SEVERITY_WEIGHTS = { + "critical": 100, + "high": 75, + "medium": 50, + "low": 25, + "info": 10 +} + +# Review verdict thresholds +VERDICT_THRESHOLDS = { + "approve": {"max_critical": 0, "max_high": 0, "max_score": 100}, + "approve_with_suggestions": {"max_critical": 0, "max_high": 2, "max_score": 85}, + "request_changes": {"max_critical": 0, "max_high": 5, "max_score": 70}, + "block": {"max_critical": float("inf"), "max_high": float("inf"), "max_score": 0} +} + + +def load_json_file(filepath: str) -> Optional[Dict]: + """Load JSON file if it exists.""" + try: + with open(filepath, "r") as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError): + return None + + +def run_pr_analyzer(repo_path: Path) -> Dict: + """Run pr_analyzer.py and return results.""" + script_path = Path(__file__).parent / "pr_analyzer.py" + if not script_path.exists(): + return {"status": "error", "message": "pr_analyzer.py not found"} + + try: + result = subprocess.run( + [sys.executable, str(script_path), str(repo_path), "--json"], + capture_output=True, + text=True, + timeout=120 + ) + if result.returncode == 0: + return json.loads(result.stdout) + return {"status": "error", "message": result.stderr} + except Exception as e: + return {"status": "error", "message": str(e)} + + +def run_quality_checker(repo_path: Path) -> Dict: + """Run code_quality_checker.py and return results.""" + script_path = Path(__file__).parent / "code_quality_checker.py" + if not script_path.exists(): + return {"status": "error", "message": "code_quality_checker.py not found"} + + try: + result = subprocess.run( + [sys.executable, str(script_path), str(repo_path), "--json"], + capture_output=True, + text=True, + timeout=300 + ) + if result.returncode == 0: + return json.loads(result.stdout) + return {"status": "error", "message": result.stderr} + except Exception as e: + return {"status": "error", "message": str(e)} + + +def calculate_review_score(pr_analysis: Dict, quality_analysis: Dict) -> int: + """Calculate overall review score (0-100).""" + score = 100 + + # Deduct for PR risks + if "risks" in pr_analysis: + risks = pr_analysis["risks"] + score -= len(risks.get("critical", [])) * 15 + score -= len(risks.get("high", [])) * 10 + score -= len(risks.get("medium", [])) * 5 + score -= len(risks.get("low", [])) * 2 + + # Deduct for code quality issues + if "issues" in quality_analysis: + issues = quality_analysis["issues"] + score -= len([i for i in issues if i.get("severity") == "critical"]) * 12 + score -= len([i for i in issues if i.get("severity") == "high"]) * 8 + score -= len([i for i in issues if i.get("severity") == "medium"]) * 4 + score -= len([i for i in issues if i.get("severity") == "low"]) * 1 + + # Deduct for complexity + if "summary" in pr_analysis: + complexity = pr_analysis["summary"].get("complexity_score", 0) + if complexity > 7: + score -= 10 + elif complexity > 5: + score -= 5 + + return max(0, min(100, score)) + + +def determine_verdict(score: int, critical_count: int, high_count: int) -> Tuple[str, str]: + """Determine review verdict based on score and issue counts.""" + if critical_count > 0: + return "block", "Critical issues must be resolved before merge" + + if score >= 90 and high_count == 0: + return "approve", "Code meets quality standards" + + if score >= 75 and high_count <= 2: + return "approve_with_suggestions", "Minor improvements recommended" + + if score >= 50: + return "request_changes", "Several issues need to be addressed" + + return "block", "Significant issues prevent approval" + + +def generate_findings_list(pr_analysis: Dict, quality_analysis: Dict) -> List[Dict]: + """Combine and prioritize all findings.""" + findings = [] + + # Add PR risk findings + if "risks" in pr_analysis: + for severity, items in pr_analysis["risks"].items(): + for item in items: + findings.append({ + "source": "pr_analysis", + "severity": severity, + "category": item.get("name", "unknown"), + "message": item.get("message", ""), + "file": item.get("file", ""), + "count": item.get("count", 1) + }) + + # Add code quality findings + if "issues" in quality_analysis: + for issue in quality_analysis["issues"]: + findings.append({ + "source": "quality_analysis", + "severity": issue.get("severity", "medium"), + "category": issue.get("type", "unknown"), + "message": issue.get("message", ""), + "file": issue.get("file", ""), + "line": issue.get("line", 0) + }) + + # Sort by severity weight + findings.sort( + key=lambda x: -SEVERITY_WEIGHTS.get(x["severity"], 0) + ) + + return findings + + +def generate_action_items(findings: List[Dict]) -> List[Dict]: + """Generate prioritized action items from findings.""" + action_items = [] + seen_categories = set() + + for finding in findings: + category = finding["category"] + severity = finding["severity"] + + # Group similar issues + if category in seen_categories and severity not in ["critical", "high"]: + continue + + action = { + "priority": "P0" if severity == "critical" else "P1" if severity == "high" else "P2", + "action": get_action_for_category(category, finding), + "severity": severity, + "files_affected": [finding["file"]] if finding.get("file") else [] + } + action_items.append(action) + seen_categories.add(category) + + return action_items[:15] # Top 15 actions + + +def get_action_for_category(category: str, finding: Dict) -> str: + """Get actionable recommendation for issue category.""" + actions = { + "hardcoded_secrets": "Remove hardcoded credentials and use environment variables or a secrets manager", + "sql_concatenation": "Use parameterized queries to prevent SQL injection", + "debugger": "Remove debugger statements before merging", + "console_log": "Remove or replace console statements with proper logging", + "todo_fixme": "Address TODO/FIXME comments or create tracking issues", + "disable_eslint": "Address the underlying issue instead of disabling lint rules", + "any_type": "Replace 'any' types with proper type definitions", + "long_function": "Break down function into smaller, focused units", + "god_class": "Split class into smaller, single-responsibility classes", + "too_many_params": "Use parameter objects or builder pattern", + "deep_nesting": "Refactor using early returns, guard clauses, or extraction", + "high_complexity": "Reduce cyclomatic complexity through refactoring", + "missing_error_handling": "Add proper error handling and recovery logic", + "duplicate_code": "Extract duplicate code into shared functions", + "magic_numbers": "Replace magic numbers with named constants", + "large_file": "Consider splitting into multiple smaller modules" + } + return actions.get(category, f"Review and address: {finding.get('message', category)}") + + +def format_markdown_report(report: Dict) -> str: + """Generate markdown-formatted report.""" + lines = [] + + # Header + lines.append("# Code Review Report") + lines.append("") + lines.append(f"**Generated:** {report['metadata']['generated_at']}") + lines.append(f"**Repository:** {report['metadata']['repository']}") + lines.append("") + + # Executive Summary + lines.append("## Executive Summary") + lines.append("") + summary = report["summary"] + verdict = summary["verdict"] + verdict_emoji = { + "approve": "✅", + "approve_with_suggestions": "✅", + "request_changes": "⚠️", + "block": "❌" + }.get(verdict, "❓") + + lines.append(f"**Verdict:** {verdict_emoji} {verdict.upper().replace('_', ' ')}") + lines.append(f"**Score:** {summary['score']}/100") + lines.append(f"**Rationale:** {summary['rationale']}") + lines.append("") + + # Issue Counts + lines.append("### Issue Summary") + lines.append("") + lines.append("| Severity | Count |") + lines.append("|----------|-------|") + for severity in ["critical", "high", "medium", "low"]: + count = summary["issue_counts"].get(severity, 0) + lines.append(f"| {severity.capitalize()} | {count} |") + lines.append("") + + # PR Statistics (if available) + if "pr_summary" in report: + pr = report["pr_summary"] + lines.append("### Change Statistics") + lines.append("") + lines.append(f"- **Files Changed:** {pr.get('files_changed', 'N/A')}") + lines.append(f"- **Lines Added:** +{pr.get('total_additions', 0)}") + lines.append(f"- **Lines Removed:** -{pr.get('total_deletions', 0)}") + lines.append(f"- **Complexity:** {pr.get('complexity_label', 'N/A')}") + lines.append("") + + # Action Items + if report.get("action_items"): + lines.append("## Action Items") + lines.append("") + for i, item in enumerate(report["action_items"], 1): + priority = item["priority"] + emoji = "🔴" if priority == "P0" else "🟠" if priority == "P1" else "🟡" + lines.append(f"{i}. {emoji} **[{priority}]** {item['action']}") + if item.get("files_affected"): + lines.append(f" - Files: {', '.join(item['files_affected'][:3])}") + lines.append("") + + # Critical Findings + critical_findings = [f for f in report.get("findings", []) if f["severity"] == "critical"] + if critical_findings: + lines.append("## Critical Issues (Must Fix)") + lines.append("") + for finding in critical_findings: + lines.append(f"- **{finding['category']}** in `{finding.get('file', 'unknown')}`") + lines.append(f" - {finding['message']}") + lines.append("") + + # High Priority Findings + high_findings = [f for f in report.get("findings", []) if f["severity"] == "high"] + if high_findings: + lines.append("## High Priority Issues") + lines.append("") + for finding in high_findings[:10]: + lines.append(f"- **{finding['category']}** in `{finding.get('file', 'unknown')}`") + lines.append(f" - {finding['message']}") + lines.append("") + + # Review Order (if available) + if "review_order" in report: + lines.append("## Suggested Review Order") + lines.append("") + for i, filepath in enumerate(report["review_order"][:10], 1): + lines.append(f"{i}. `{filepath}`") + lines.append("") + + # Footer + lines.append("---") + lines.append("*Generated by Code Reviewer*") + + return "\n".join(lines) + + +def format_text_report(report: Dict) -> str: + """Generate plain text report.""" + lines = [] + + lines.append("=" * 60) + lines.append("CODE REVIEW REPORT") + lines.append("=" * 60) + lines.append("") + lines.append(f"Generated: {report['metadata']['generated_at']}") + lines.append(f"Repository: {report['metadata']['repository']}") + lines.append("") + + summary = report["summary"] + verdict = summary["verdict"].upper().replace("_", " ") + lines.append(f"VERDICT: {verdict}") + lines.append(f"SCORE: {summary['score']}/100") + lines.append(f"RATIONALE: {summary['rationale']}") + lines.append("") + + lines.append("--- ISSUE SUMMARY ---") + for severity in ["critical", "high", "medium", "low"]: + count = summary["issue_counts"].get(severity, 0) + lines.append(f" {severity.capitalize()}: {count}") + lines.append("") + + if report.get("action_items"): + lines.append("--- ACTION ITEMS ---") + for i, item in enumerate(report["action_items"][:10], 1): + lines.append(f" {i}. [{item['priority']}] {item['action']}") + lines.append("") + + critical = [f for f in report.get("findings", []) if f["severity"] == "critical"] + if critical: + lines.append("--- CRITICAL ISSUES ---") + for f in critical: + lines.append(f" [{f.get('file', 'unknown')}] {f['message']}") + lines.append("") + + lines.append("=" * 60) + + return "\n".join(lines) + + +def generate_report( + repo_path: Path, + pr_analysis: Optional[Dict] = None, + quality_analysis: Optional[Dict] = None +) -> Dict: + """Generate comprehensive review report.""" + # Run analyses if not provided + if pr_analysis is None: + pr_analysis = run_pr_analyzer(repo_path) + + if quality_analysis is None: + quality_analysis = run_quality_checker(repo_path) + + # Generate findings + findings = generate_findings_list(pr_analysis, quality_analysis) + + # Count issues by severity + issue_counts = { + "critical": len([f for f in findings if f["severity"] == "critical"]), + "high": len([f for f in findings if f["severity"] == "high"]), + "medium": len([f for f in findings if f["severity"] == "medium"]), + "low": len([f for f in findings if f["severity"] == "low"]) + } + + # Calculate score and verdict + score = calculate_review_score(pr_analysis, quality_analysis) + verdict, rationale = determine_verdict( + score, + issue_counts["critical"], + issue_counts["high"] + ) + + # Generate action items + action_items = generate_action_items(findings) + + # Build report + report = { + "metadata": { + "generated_at": datetime.now().isoformat(), + "repository": str(repo_path), + "version": "1.0.0" + }, + "summary": { + "score": score, + "verdict": verdict, + "rationale": rationale, + "issue_counts": issue_counts + }, + "findings": findings, + "action_items": action_items + } + + # Add PR summary if available + if pr_analysis.get("status") == "analyzed": + report["pr_summary"] = pr_analysis.get("summary", {}) + report["review_order"] = pr_analysis.get("review_order", []) + + # Add quality summary if available + if quality_analysis.get("status") == "analyzed": + report["quality_summary"] = quality_analysis.get("summary", {}) + + return report -class ReviewReportGenerator: - """Main class for review report generator functionality""" - - def __init__(self, target_path: str, verbose: bool = False): - self.target_path = Path(target_path) - self.verbose = verbose - self.results = {} - - def run(self) -> Dict: - """Execute the main functionality""" - print(f"🚀 Running {self.__class__.__name__}...") - print(f"📁 Target: {self.target_path}") - - try: - self.validate_target() - self.analyze() - self.generate_report() - - print("✅ Completed successfully!") - return self.results - - except Exception as e: - print(f"❌ Error: {e}") - sys.exit(1) - - def validate_target(self): - """Validate the target path exists and is accessible""" - if not self.target_path.exists(): - raise ValueError(f"Target path does not exist: {self.target_path}") - - if self.verbose: - print(f"✓ Target validated: {self.target_path}") - - def analyze(self): - """Perform the main analysis or operation""" - if self.verbose: - print("📊 Analyzing...") - - # Main logic here - self.results['status'] = 'success' - self.results['target'] = str(self.target_path) - self.results['findings'] = [] - - # Add analysis results - if self.verbose: - print(f"✓ Analysis complete: {len(self.results.get('findings', []))} findings") - - def generate_report(self): - """Generate and display the report""" - print("\n" + "="*50) - print("REPORT") - print("="*50) - print(f"Target: {self.results.get('target')}") - print(f"Status: {self.results.get('status')}") - print(f"Findings: {len(self.results.get('findings', []))}") - print("="*50 + "\n") def main(): - """Main entry point""" parser = argparse.ArgumentParser( - description="Review Report Generator" + description="Generate comprehensive code review reports" ) parser.add_argument( - 'target', - help='Target path to analyze or process' + "repo_path", + nargs="?", + default=".", + help="Path to repository (default: current directory)" ) parser.add_argument( - '--verbose', '-v', - action='store_true', - help='Enable verbose output' + "--pr-analysis", + help="Path to pre-computed PR analysis JSON" ) parser.add_argument( - '--json', - action='store_true', - help='Output results as JSON' + "--quality-analysis", + help="Path to pre-computed quality analysis JSON" ) parser.add_argument( - '--output', '-o', - help='Output file path' + "--format", "-f", + choices=["text", "markdown", "json"], + default="text", + help="Output format (default: text)" ) - - args = parser.parse_args() - - tool = ReviewReportGenerator( - args.target, - verbose=args.verbose + parser.add_argument( + "--output", "-o", + help="Write output to file" + ) + parser.add_argument( + "--json", + action="store_true", + help="Output as JSON (shortcut for --format json)" ) - - results = tool.run() - - if args.json: - output = json.dumps(results, indent=2) - if args.output: - with open(args.output, 'w') as f: - f.write(output) - print(f"Results written to {args.output}") - else: - print(output) -if __name__ == '__main__': + args = parser.parse_args() + + repo_path = Path(args.repo_path).resolve() + if not repo_path.exists(): + print(f"Error: Path does not exist: {repo_path}", file=sys.stderr) + sys.exit(1) + + # Load pre-computed analyses if provided + pr_analysis = None + quality_analysis = None + + if args.pr_analysis: + pr_analysis = load_json_file(args.pr_analysis) + if not pr_analysis: + print(f"Warning: Could not load PR analysis from {args.pr_analysis}") + + if args.quality_analysis: + quality_analysis = load_json_file(args.quality_analysis) + if not quality_analysis: + print(f"Warning: Could not load quality analysis from {args.quality_analysis}") + + # Generate report + report = generate_report(repo_path, pr_analysis, quality_analysis) + + # Format output + output_format = "json" if args.json else args.format + + if output_format == "json": + output = json.dumps(report, indent=2) + elif output_format == "markdown": + output = format_markdown_report(report) + else: + output = format_text_report(report) + + # Write or print output + if args.output: + with open(args.output, "w") as f: + f.write(output) + print(f"Report written to {args.output}") + else: + print(output) + + +if __name__ == "__main__": main()