roboco/docs/standards/architecture/code_review.md

Code Review Guidelines

Standards for conducting effective code reviews in the RoboCo system.

---

Table of Contents

1. Review Philosophy
2. Reviewer Responsibilities
3. Author Responsibilities
4. Review Checklist
5. Feedback Guidelines
6. Severity Classification
7. Common Issues
8. Automated Checks

---

Review Philosophy

CR-001: Purpose of Code Review

Goals:

1. Catch bugs - Find defects before they reach production
2. Maintain quality - Ensure code meets standards
3. Share knowledge - Spread understanding across team
4. Improve design - Identify better approaches
5. Ensure consistency - Keep codebase uniform

NOT goals:

• Demonstrate superiority
• Nitpick style (that's what linters are for)
• Rewrite someone's code
• Block progress indefinitely

CR-002: Review Mindset

As Reviewer:

• Assume the author did their best
• Ask questions before making assumptions
• Explain the "why" behind suggestions
• Be constructive, not destructive
• Praise good work

As Author:

• Code review is about the code, not you
• Every suggestion is an opportunity to learn
• Explain your reasoning when disagreeing
• Thank reviewers for their time

---

Reviewer Responsibilities

CR-010: Response Time

Priority	First Response	Full Review
Urgent (blocker fix)	< 2 hours	< 4 hours
Normal	< 4 hours	< 1 day
Low (refactor, docs)	< 1 day	< 2 days

CR-011: Review Depth

Quick Pass (5 min):

• Does the PR description make sense?
• Are tests included?
• Does CI pass?

Thorough Review (30+ min):

• Understand the full context
• Check logic and edge cases
• Verify tests cover scenarios
• Review documentation updates

CR-012: What to Review

Must Review	Should Review	Don't Review
Logic correctness	Code style	Auto-generated code
Error handling	Performance	Formatting (linter handles)
Security concerns	Naming	Import order (linter handles)
Test coverage	Documentation
API contracts	Code organization

---

Author Responsibilities

CR-020: Before Requesting Review

Pre-submission Checklist:

- [ ] All automated checks pass (lint, type check, tests)
- [ ] Self-reviewed the diff
- [ ] PR description explains the change
- [ ] Tests cover new functionality
- [ ] Documentation updated if needed
- [ ] No debugging code left in
- [ ] No unrelated changes included
- [ ] Commit history is clean

CR-021: Writing Good PR Descriptions

Template:

## Summary
Brief description of what this PR does.

## Changes
- Added X functionality
- Modified Y behavior
- Removed deprecated Z

## Testing
How to test this change:
1. Step one
2. Step two
3. Expected result

## Related
- Task: TASK-123
- Related PR: #456

CR-022: Keeping PRs Small

Size Guidelines:

Lines Changed	Classification	Review Time
< 100	Small	15 min
100-300	Medium	30 min
300-500	Large	1 hour
> 500	Too Large	Split it!

How to Split Large PRs:

1. By layer: API → Service → Repository
2. By feature: Core logic → Edge cases → Polish
3. By concern: Main feature → Tests → Docs

---

Review Checklist

CR-030: Functionality

- [ ] Code does what PR description says
- [ ] Edge cases are handled
- [ ] Error conditions are handled gracefully
- [ ] No obvious bugs or logic errors
- [ ] Performance is acceptable for use case

CR-031: Code Quality

- [ ] Follows project coding standards
- [ ] No code duplication (DRY)
- [ ] Functions/classes have single responsibility
- [ ] Naming is clear and consistent
- [ ] Comments explain "why", not "what"
- [ ] No dead code or TODOs without context

CR-032: Security

- [ ] No hardcoded secrets or credentials
- [ ] User input is validated and sanitized
- [ ] SQL queries use parameterized statements
- [ ] No command injection vulnerabilities
- [ ] Sensitive data is not logged
- [ ] Access control is properly enforced

CR-033: Testing

- [ ] New code has tests
- [ ] Tests cover happy path
- [ ] Tests cover error cases
- [ ] Tests are readable and maintainable
- [ ] No tests skipped without reason
- [ ] Mocking is appropriate (not excessive)

CR-034: API Design

- [ ] API is intuitive and consistent
- [ ] Breaking changes are noted
- [ ] Error responses are informative
- [ ] Documentation is updated
- [ ] Backwards compatibility maintained

CR-035: Data Handling

- [ ] Database migrations are reversible
- [ ] Indexes are used appropriately
- [ ] No N+1 query issues
- [ ] Large data sets are handled efficiently
- [ ] Transactions are used correctly

---

Feedback Guidelines

CR-040: How to Give Feedback

Structure:

[Severity]: [Issue]

[Context/Reason]

[Suggestion if applicable]

Examples:

# Good feedback
BLOCKER: This SQL query is vulnerable to injection

The user input is concatenated directly. An attacker could
extract all data with: `'; DROP TABLE users; --`

Suggestion: Use parameterized queries:
```python
await db.execute("SELECT * FROM users WHERE id = :id", {"id": user_id})

Bad feedback

"This is wrong."

### CR-041: Severity Prefixes

Use prefixes to indicate urgency:

| Prefix | Meaning | Action |
|--------|---------|--------|
| `BLOCKER:` | Must fix, security/correctness issue | Cannot merge |
| `MAJOR:` | Should fix, significant concern | Should address |
| `MINOR:` | Nice to fix, improvement | Can defer |
| `NIT:` | Nitpick, style preference | Optional |
| `QUESTION:` | Need clarification | Explain |
| `PRAISE:` | Good work | Keep doing this! |

### CR-042: Types of Comments

**Actionable:**
```markdown
MAJOR: This function modifies its input parameter, which can cause
unexpected behavior for callers. Consider returning a new object instead.

Question (non-blocking):

QUESTION: Is this timeout intentionally set to 5 minutes? Seems long
for a health check.

Suggestion (optional):

NIT: Could use list comprehension here for readability:
`[x.name for x in items if x.active]`

Praise:

PRAISE: Great error handling here! The retry logic with backoff
is exactly what we need for this external API.

CR-043: What NOT to Do

Avoid:

• Personal attacks: "Who wrote this garbage?"
• Vague criticism: "This is confusing"
• Style debates: "I prefer X" (unless it violates standards)
• Demands without explanation: "Change this"
• Blocking for non-issues: Minor style preferences

---

Severity Classification

CR-050: Severity Definitions

Severity	Definition	Examples
BLOCKER	Security vulnerability, data loss risk, breaks build	SQL injection, missing auth check, crashes
MAJOR	Significant bug, performance issue, design flaw	Logic error, N+1 queries, missing validation
MINOR	Improvement opportunity, minor bug	Better naming, missing edge case, documentation
NIT	Style preference, optional enhancement	Alternative approach, formatting preference

CR-051: Blocking vs Non-Blocking

Block merge for:

• Security vulnerabilities (any severity)
• Logic errors that affect functionality
• Missing tests for critical paths
• Breaking API changes without migration
• Violations of ERROR-level coding standards

Don't block merge for:

• Style preferences covered by linters
• "I would have done it differently"
• Missing documentation (unless API change)
• Code that works but isn't "perfect"
• Minor optimizations

---

Common Issues

CR-060: Logic Issues

Missing null checks:

# Bad
user.name.lower()  # What if user is None?

# Good
if user and user.name:
    user.name.lower()

Off-by-one errors:

# Bad
for i in range(len(items) + 1):  # IndexError on last iteration
    items[i]

# Good
for i in range(len(items)):
    items[i]

Race conditions:

# Bad
if task.status == "pending":
    # Another process could change status here!
    task.status = "claimed"
    db.save(task)

# Good - Use atomic operations
await db.execute(
    "UPDATE tasks SET status = 'claimed' WHERE id = :id AND status = 'pending'",
    {"id": task.id}
)

CR-061: Performance Issues

N+1 queries:

# Bad
tasks = await db.query(Task).all()
for task in tasks:
    owner = await db.query(User).filter_by(id=task.owner_id).first()  # N queries!

# Good
tasks = await db.query(Task).options(selectinload(Task.owner)).all()

Unbounded queries:

# Bad
users = await db.query(User).all()  # Could be millions

# Good
users = await db.query(User).limit(100).all()

Memory bloat:

# Bad
data = [x for x in huge_iterator]  # Loads all into memory

# Good
for x in huge_iterator:  # Process one at a time
    process(x)

CR-062: Security Issues

SQL injection:

# Bad
f"SELECT * FROM users WHERE id = '{user_input}'"

# Good
"SELECT * FROM users WHERE id = :id", {"id": user_input}

Missing authorization:

# Bad
@router.delete("/tasks/{task_id}")
async def delete_task(task_id: str) -> None:
    await db.delete_task(task_id)  # Anyone can delete any task!

# Good
@router.delete("/tasks/{task_id}")
async def delete_task(task_id: str, current_user: User = Depends()) -> None:
    task = await db.get_task(task_id)
    if task.owner_id != current_user.id:
        raise HTTPException(403, "Not authorized")
    await db.delete_task(task_id)

Sensitive data exposure:

# Bad
logger.info(f"User login: {user.email}, password: {password}")

# Good
logger.info("User login", user_id=user.id)

---

Automated Checks

CR-070: Required Checks

All PRs must pass these automated checks before merge:

Check	Tool	Purpose
Formatting	ruff format	Code style consistency
Linting	ruff check	Code quality issues
Type checking	mypy	Type safety
Tests	pytest	Functionality verification
Dead code	vulture	Remove unused code
Security	bandit	Security vulnerabilities
Complexity	xenon	Maintainability

CR-071: CI Pipeline

# .github/workflows/ci.yml
jobs:
  check:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: astral-sh/setup-uv@v4

      - name: Format check
        run: uv run ruff format --check .

      - name: Lint
        run: uv run ruff check .

      - name: Type check
        run: uv run mypy roboco/

      - name: Tests
        run: uv run pytest --cov=roboco --cov-fail-under=80

      - name: Security scan
        run: uv run bandit -r roboco/ -ll

CR-072: Pre-commit Hooks

Use pre-commit hooks to catch issues early:

# .pre-commit-config.yaml
repos:
  - repo: local
    hooks:
      - id: ruff-format
        name: ruff format
        entry: uv run ruff format
        language: system
        types: [python]

      - id: ruff-check
        name: ruff check
        entry: uv run ruff check --fix
        language: system
        types: [python]

      - id: mypy
        name: mypy
        entry: uv run mypy
        language: system
        types: [python]

---

Quick Reference

Review Flow

1. Author creates PR
   └─► Auto-checks run

2. Reviewer assigned
   └─► Quick pass (5 min)
       └─► Issues? Request changes early

3. Full review
   └─► Check functionality
   └─► Check code quality
   └─► Check security
   └─► Check tests

4. Feedback given
   └─► BLOCKER/MAJOR: Must address
   └─► MINOR/NIT: Optional

5. Author addresses feedback
   └─► Push changes
   └─► Reply to comments

6. Re-review if needed
   └─► Approve or request more changes

7. Merge
   └─► Delete branch

Comment Templates

Blocker:

BLOCKER: [Brief issue]

[Why this is a problem]

[How to fix it]

Question:

QUESTION: [What you don't understand]

[Context for why you're asking]

Praise:

PRAISE: [What's good about this]

[Why it's particularly good]

Time Estimates

PR Size	Lines	Review Time
XS	< 50	10 min
S	50-100	15 min
M	100-300	30 min
L	300-500	1 hour
XL	> 500	Split it!