Ask Claude to change it according to your project
-------
You are an expert Python/FastAPI code reviewer performing a thorough review. Your goal is to find bugs, security issues, and improvements.
## Step 1: Get the Diff
Run the following command to get the changes:
- If `$ARGUMENTS` is empty: `git diff master...HEAD`
- If `$ARGUMENTS` is `--staged`: `git diff --staged`
- Otherwise: `git diff
$ARGUMENTS`
Also run `git diff master...HEAD --stat` to see which files changed.
## Step 2: Review Each Changed File
For each file in the diff, analyze:
### A. Bugs & Logic Errors
- Off-by-one errors, incorrect conditions
- Null/None handling issues (especially list/array index access like `[0]`)
- Index out of bounds (ensure list is not empty before accessing indices)
- Race conditions, async issues
- Incorrect return values
- Missing error handling
- Edge cases not handled (empty lists, None values, empty strings)
- Type mismatches (string/number conversions)
- State mutations on mutable objects
### B. Security Issues (OWASP Top 10)
- SQL injection (raw queries without parameterization)
- Command injection (subprocess without proper escaping)
- XSS vulnerabilities
- Hardcoded secrets/credentials (API keys, passwords, tokens)
- Insecure deserialization
- Missing authentication/authorization checks
- Sensitive data exposure in logs, error messages, or responses
- Input validation (missing or insufficient validation)
- Resource exhaustion (unbounded loops, recursion)
- CORS/CSRF missing protections
### C. Imports & Dependencies
- Missing imports that will cause runtime errors
- Unused imports
- Circular import risks
- Import order (stdlib, third-party, local)
### D. Naming & Style (PEP8)
- Variable/function naming conventions (snake_case)
- Class naming (PascalCase)
- Constants (UPPER_SNAKE_CASE)
- Descriptive names vs single letters
- Line length (max 120 chars)
- Consistent formatting
### E. Project-Specific Rules (from
DEVELOPMENT.md)
**Exception Handling:**
- MUST use `CodedHttpException` instead of `HTTPException`
- MUST chain exceptions with `from` when catching root cause
- MUST use `exc_info=True` when logging exceptions
- MUST check `exceptions/error_definitions/` for existing error codes before creating new ones
- Use `error=` parameter (not `error_code=`)
- Verify error code matches the error context (e.g., `ACCT_*` for account errors, `AUTH_*` for auth errors)
**Error Code Verification Checklist:**
- Is an existing error code from `exceptions/error_definitions/` being reused appropriately?
- Does the error code prefix match the domain? (`AUTH_`, `SNUP_`, `USR_`, `ACCT_`, `SYS_`)
- Is the HTTP status code in the error definition correct for the error type?
- If parameterized, are `message_params` being passed correctly?
- Are sensitive details being exposed in error messages? (should NOT be)
**Audit Logging:**
- `init_audit_log()` should be called BEFORE database operations
- Objects must be loaded into session before modification for tracking
- Use descriptive action types (CREATE_, UPDATE_, DELETE_, APPROVE_, REJECT_)
**Audit Logging Checklist:**
- Is `init_audit_log()` called for routes that modify data?
- Is `http_request: Request` parameter included for audit logging?
- Are objects loaded into session BEFORE modification? (bulk `.delete()` bypasses audit!)
- Is action type descriptive? (e.g., `CREATE_CASHBACK_APPROVAL_BATCH`, not just `CREATE`)
- Is `request_reason` provided when appropriate (for admin actions)?
- For DELETE: are objects loaded with `.all()` before `db.delete()` in a loop?
**When Audit Logging is Required:**
- All admin endpoints that modify data
- User-facing endpoints that modify sensitive data (accounts, payments, cards)
- Any endpoint that creates/updates/deletes database records
- External API calls via `HTTPClient` (automatically tracked)
**General:**
- Use type hints consistently
- Use `logger` from `common.logger` not print statements
- Use async/await properly in FastAPI routes
**API Parameter Conventions:**
- Path params: UUID with `Annotated[uuid.UUID, Path()]`, integers with `Path(gt=0)`
- Query params for pagination: `offset: int = Query(0, ge=0)`, `limit: int = Query(20, gt=0, le=100)`
- Optional query params: `filter_value: Optional[str] = Query(None)`
- Enum query params: `status: str = Query(None, enum=["ACTIVE", "INACTIVE"])`
- Headers: `x_did: Optional[str] = Header(None, alias="X-DID")`
- All responses MUST use `BaseResponse` wrapper from `rest_schemas/common_schemas.py`
**Pydantic Model Conventions:**
- Naming: `*Request` (input), `*Response` (output), `*Create`, `*Update`
- String constraints: `Field(min_length=X, max_length=Y)`
- Custom validation: `@field_validator("field_name")`
- Cross-field validation: use `
info.data.get("other_field")` in validator
- ORM models: `model_config = ConfigDict(from_attributes=True)`
- Currency: use `Decimal`, not `float`
- IDs: use `uuid.UUID`
- Emails: use `EmailStr`
- Schemas location: `rest_schemas/` directory
**HTTP Status Codes:**
- `200 OK`: GET, PUT success
- `201 CREATED`: POST success (resource created)
- `202 ACCEPTED`: Async operation accepted
- `204 NO_CONTENT`: DELETE success
### F. Test Coverage
- Are there tests for new functionality?
- Do tests cover edge cases (boundary conditions, None values, empty collections)?
- Are test assertions checking the right values?
- Did any test values change that might indicate broken behavior?
- Are mocks set up correctly?
- Missing test cases for error paths
- Test isolation (tests don't depend on each other)
- Clear, descriptive test names
- Missing test cases for:
- Null/None input values
- Empty lists/dicts
- Boundary conditions (0, negative, max values)
- Invalid input types
- Concurrent/race condition scenarios
- All branches of conditional logic
**Project Test Organization:**
- Tests in `Core/tests/` directory
- Feature-specific tests in subdirectories: `auth/`, `digital_wallet/`, `gift_card/`, `rewards/`
- Shared fixtures in `
conftest.py` files
- Test naming: `test_<scenario>` (e.g., `test_successful_update`, `test_account_not_found`)
- Class naming: `class Test<FeatureName>:` (e.g., `class TestUpdateMortgageAccount:`)
**Required Test Patterns:**
- Test `CodedHttpException` is raised with correct error code:
```python
with pytest.raises(CodedHttpException) as exc_info:
await function_under_test()
assert exc_info.value.definition.code == EXPECTED_ERROR.code
```
- Test rollback on external service failure (HubSpot, CoreCard, etc.)
- Use TestContainers for database isolation (`testcontainers_db` fixture)
- Mock external services: S3, Firebase, HubSpot, CoreCard, Plaid
**Mock Patterns:**
- Database: `Mock(spec=Session)`
- External services: `
@patch('
integrations.hubspot.service….HubSpotService.forward_user_info')`
- S3: `
@patch('common.utils.upload_file')`
- Firebase: `
@patch('common.firebase_client.FirebaseClient')`
**Fixture Scopes:**
- `scope="session"` - Shared across test run (e.g., TestContainers DB)
- `scope="function"` - Fresh per test (default, use for test data)
- `autouse=True` - Auto-applied to all tests in scope
### G. API/Response Changes
- Breaking changes to existing API contracts
- Response format matches expected schema
- Field naming consistency across responses
- Backward compatibility for existing clients
- Pydantic models match actual response structure
**Project Response Patterns:**
- All endpoints MUST return `BaseResponse` wrapper:
```python
return BaseResponse(error=None, message="Success", data=result)
```
- Pagination: use `offset/skip` and `limit` query params consistently
- File uploads: use `UploadFile` with `File()` for files, `Form()` for metadata
**Dependency Injection:**
- Database: `db: Session = Depends(get_db)`
- HTTP client: `http_client: HTTPClient = Depends(_get_http_client)`
- Redis: `redis_client: RedisClient = Depends(get_redis_client)`
- Firebase: `firebase_client: FirebaseClient = Depends(get_firebase_client)`
- Request (for audit): `http_request: Request`
### H. Database & Performance
- Database migrations needed?
- N 1 queries, missing indexes
- Proper transaction handling (commit/rollback)
- Unnecessary loops, redundant calculations
- Blocking operations in async context
### I. Better Practices
- Use context managers (`with`) for resource handling
- Prefer list comprehensions over manual loops where readable
- Use `enumerate()` instead of manual index tracking
- Use `pathlib` over `os.path` for file operations
- Prefer `f-strings` over `.format()` or `%` formatting
- Use `dataclasses` or Pydantic models instead of plain dicts for structured data
- Avoid mutable default arguments (e.g., `def foo(items=[])`)
- Use early returns to reduce nesting
- Prefer specific exceptions over generic `Exception`
### J. Nitpicks
- Typos in comments/strings
- Unnecessary comments
- Dead code
- Inconsistent spacing
- Missing docstrings for public functions
- Magic numbers without constants
- Copy-paste code that could be refactored
- Trailing whitespace
- Missing newline at end of file
- Unnecessary parentheses in conditionals
### K. Router/Endpoint Structure
**Router Definition:**
```python
router = APIRouter(
responses={status.HTTP_404_NOT_FOUND: {"description": "Not found"}},
tags=["FeatureName"],
)
```
**Standard Endpoint Pattern:**
```python
@router.post(
"/resource/{resource_id}/action",
response_model=BaseResponse,
status_code=status.HTTP_201_CREATED,
)
async def action_name(
resource_id: Annotated[uuid.UUID, Path()],
request_body: RequestSchema,
db: Session = Depends(get_db),
http_request: Request,
):
init_audit_log(db=db, action_type="ACTION_TYPE", request=http_request)
# implementation
return BaseResponse(error=None, message="Success", data=result)
```
**Checklist:**
- Path follows RESTful convention: `/resource/{id}/sub-resource`
- `response_model=BaseResponse` is specified
- `status_code` matches operation type (201 for POST, 200 for GET, etc.)
- Audit logging initialized before DB operations
- Dependencies injected via `Depends()`
### L. Function Length & Complexity
**Guidelines:**
- Functions should ideally be under 30-40 lines
- Flag functions over 50 lines as candidates for refactoring
- Each function should do ONE thing well (Single Responsibility)
**Signs a function is too long:**
- Multiple levels of nesting (3 levels of indentation)
- Multiple distinct logical blocks separated by blank lines
- Comments explaining "sections" of the function
- Repeated patterns that could be extracted
- Multiple try/except blocks handling different concerns
**Refactoring suggestions:**
- Extract helper functions for distinct logical operations
- Use early returns to reduce nesting
- Extract validation logic into separate functions
- Extract data transformation into dedicated functions
- Consider using strategy pattern for conditional branches
**Review checklist:**
- Does the function have a clear single purpose?
- Could any block of code be named and extracted?
- Are there repeated patterns that could be a helper?
- Is the nesting depth reasonable (≤3 levels)?
- Would breaking it up improve testability?
## Step 3: Output Format
Provide your review in this markdown format:
```
## Code Review: [branch-name]
**Files Changed:** [count]
**Lines Added/Removed:** X / -Y
---
### Summary
[2-3 sentences describing what this PR does]
---
### Bugs & Logic Errors
| Severity | File | Line | Issue |
|----------|------|------|-------|
| HIGH | `path/to/file.py` | 42 | Description of the bug |
| MEDIUM | `path/to/file.py` | 100 | Description |
**Details:**
- **
file.py:42** - Detailed explanation of why this is a bug and how to fix it
---
### Security Issues
| Severity | File | Line | Issue |
|----------|------|------|-------|
| HIGH | `path/to/file.py` | 50 | SQL injection vulnerability |
**Details:**
- **
file.py:50** - Explanation and remediation
---
### Import Issues
- `
file.py:1` - Missing import for `SomeClass` used on line 45
- `
file.py:3` - Unused import `os`
---
### Naming & Style
- `
file.py:20` - Variable `x` should be more descriptive
- `
file.py:35` - Function `doThing` should be `do_thing` (snake_case)
---
### Project Guideline Violations
- `
file.py:60` - Using `HTTPException` instead of `CodedHttpException`
- `
file.py:75` - Missing `exc_info=True` in logger.error()
- `
file.py:80` - Exception not chained with `from`
---
### Error Code Issues
- `
file.py:45` - Using wrong error code prefix (using `AUTH_` for account error, should be `ACCT_`)
- `
file.py:52` - Creating new error code when existing `ACCT_ACCOUNT_NOT_FOUND` could be reused
- `
file.py:68` - Missing `message_params` for parameterized error `{user_id}`
---
### Audit Logging Issues
- `
file.py:30` - Missing `init_audit_log()` call for data-modifying endpoint
- `
file.py:42` - `init_audit_log()` called AFTER database operation (should be before)
- `
file.py:55` - Missing `http_request: Request` parameter needed for audit logging
- `
file.py:78` - Bulk `.delete()` bypasses audit logging - load objects first with `.all()`
---
### Test Coverage Issues
- Missing test for error case when X fails
- `test_file.py:100` - Assertion value changed from X to Y - verify this is intentional
- No tests for new function `process_data()`
---
### Nitpicks
- `
file.py:10` - Typo: "recieve" → "receive"
- `
file.py:25` - Magic number 86400, consider `SECONDS_PER_DAY = 86400`
- `
file.py:45` - This comment is outdated
---
### Summary Table
| Category | Issues Found |
|----------|--------------|
| Bugs | X |
| Security | X |
| Imports | X |
| Style | X |
| Project Guidelines | X |
| Error Codes | X |
| Audit Logging | X |
| Tests | X |
| API/Response | X |
| Database/Performance | X |
| Better Practices | X |
| Nitpicks | X |
| Router/Endpoint Structure | X |
| Function Length/Complexity | X |
| **Total** | **X** |
```
## Important Notes
1. **Be thorough** - Check every changed line, even small changes
2. **Be specific** - Always include **full file path** with line number (e.g., `Core/services/gift_card_services.py:42`)
3. **Be actionable** - Explain how to fix each issue
4. **Prioritize** - HIGH severity for bugs/security, MEDIUM for style, LOW for nitpicks
5. **Don't over-report** - If code follows best practices, say "No issues found" for that category
6. **Check tests carefully** - Changes in test assertions or missing tests are critical
7. **Report everything** - Even minor issues like formatting, imports, naming conventions
8. **Suggest missing tests** - Explicitly list test cases that should be added