eryao/.trellis/spec/backend/quality-guidelines.md

# Quality Guidelines

> Code quality standards for backend development.

---

## Overview

This project enforces quality through:

- **Linting**: `ruff` (fast Python linter)
- **Type checking**: `basedpyright` (strict type checker)
- **Formatting**: Consistent code style via linter rules
- **Testing**: `pytest` with async support (`pytest-asyncio`)
- **Pre-commit hooks**: Automated checks before commits

---

## Forbidden Patterns

### ❌ Bypassing Lint/Type Gates

```python
# WRONG: Suppress linter warnings
result = some_function()  # noqa: BLE001, reportImplicitRelativeImport
```

**Right way:** Fix the underlying issue:

```python
# Fix exception handling
except Exception as exc:
    logger.error("operation_failed", error=str(exc))
    raise
```

### ❌ Using `print()` in Runtime Code

```python
# WRONG: Never use print in production code
print(f"Processing user {user_id}")
```

**Right way:** Use `core.logging`:

```python
from core.logging import get_logger

logger = get_logger(__name__)
logger.info("processing_user", user_id=user_id)
```

### ❌ Hardcoding Secrets

```python
# WRONG: Hardcode credentials
api_key = "sk-abcd1234..."
password = "postgres123"
```

**Right way:** Use `core.config.settings`:

```python
from core.config.settings import config

api_key = config.llm.provider_keys["openai"]
password = config.database.password
```

### ❌ Manual Environment Parsing

```python
# WRONG: Direct os.getenv
import os
db_host = os.getenv("DATABASE_HOST", "localhost")
```

**Right way:** Use `Settings`:

```python
from core.config.settings import config

db_host = config.database.host
```

### ❌ Silent Exception Handling

```python
# WRONG: Swallow exception
try:
    await service.do_something()
except Exception:
    pass  # Destroys debuggability
```

**Right way:** Log and propagate:

```python
try:
    await service.do_something()
except Exception as exc:
    logger.error("operation_failed", error=str(exc), exc_info=True)
    raise
```

---

## Required Patterns

### ✅ Strong Typing at Boundaries

```python
from pydantic import BaseModel

# Request/response schemas must use Pydantic
class CreateSessionRequest(BaseModel):
    thread_id: str
    messages: list[Message]

class SessionResponse(BaseModel):
    id: UUID
    owner_id: UUID
    created_at: datetime
```

### ✅ Schema-First for Data Contracts

```python
# Define schema first, then implement
# v1/agent/schemas.py
class TaskAccepted(BaseModel):
    run_id: str
    thread_id: str
    accepted_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))

# Then use in router
@router.post("/run", response_model=TaskAccepted)
async def create_run(...):
    return TaskAccepted(run_id=run_id, thread_id=thread_id)
```

### ✅ Dependency Injection via FastAPI

```python
# v1/agent/dependencies.py
from fastapi import Depends
from core.db.base_repository import BaseRepository

async def get_agent_service(
    session: AsyncSession = Depends(get_session),
) -> AgentService:
    repository = AgentRepository(session)
    return AgentService(repository=repository, ...)

# v1/agent/router.py
@router.post("/run")
async def create_run(
    service: AgentService = Depends(get_agent_service),
):
    return await service.enqueue_run(...)
```

### ✅ Configuration via Settings

```python
# All config through Settings
from core.config.settings import config

class SomeService:
    def __init__(self):
        self.redis_url = config.redis.url
        self.db_url = config.database.url
        self.log_level = config.runtime.log_level
```

### ✅ Logging with Context

```python
# Always include relevant context
logger.info(
    "run_completed",
    run_id=run_id,
    thread_id=thread_id,
    duration_seconds=duration,
    user_id=current_user.id,
)
```

---

## Testing Requirements

### Test Organization

```
backend/tests/
├── unit/              # Unit tests (no external dependencies)
├── integration/       # Integration tests (DB, Redis, HTTP)
├── conftest.py        # Shared fixtures
└── ...
```

### Test Requirements

**From AGENTS.md:**
1. **TDD when practical**: Write tests before/alongside implementation
2. **Regression tests**: Changed logic/contracts must have regression tests
3. **Real DB tests**: Use `settings.test.*` credentials (never hardcode)
4. **Integration tests**: Start backend via `./infra/scripts/app.sh` before running
5. **Restart for integration**: Use `./infra/scripts/app.sh restart` before `uv run pytest`

### Test Configuration

```python
# pyproject.toml
[tool.pytest.ini_options]
testpaths = ["backend/tests"]
addopts = "-q --import-mode=importlib"
asyncio_mode = "auto"
pythonpath = ["backend/src"]
```

### Example Test

```python
import pytest
from httpx import AsyncClient
from core.config.settings import Settings

@pytest.mark.asyncio
async def test_create_session(settings: Settings):
    # Use settings.test.* for real DB tests
    assert settings.test.phone != ""
    
    async with AsyncClient() as client:
        response = await client.post(
            f"{settings.supabase.url}/api/v1/agent/run",
            json={"thread_id": "test-thread"},
        )
        assert response.status_code == 200
```

---

## Code Review Checklist

### Architecture

- [ ] Follows `schema -> repository -> service` layering
- [ ] Router handles HTTP only, service handles business logic
- [ ] Repository does not make auth decisions
- [ ] Transaction boundary is in service layer, not repository

### Security

- [ ] `owner_id` comes from `current_user.id` (JWT sub claim), not client payload
- [ ] No hardcoded secrets/passwords/tokens
- [ ] Sensitive fields are not logged (passwords, tokens, PII)
- [ ] Auth checks use `ApiProblemError` with proper error codes

### Data

- [ ] Database changes use Alembic migrations
- [ ] Soft delete uses `deleted_at`, queries filter deleted records
- [ ] Timezone-aware timestamps: `datetime.now(timezone.utc)`
- [ ] Foreign keys follow `<entity>_id` naming

### Error Handling

- [ ] All exceptions are logged before re-raising
- [ ] Business logic uses `ApiProblemError`, not `HTTPException`
- [ ] Error responses use RFC 7807 format with `code` field
- [ ] New error codes documented in `docs/protocols/common/http-error-codes.md`

### Logging

- [ ] Uses `core.logging.get_logger`, not `print()`
- [ ] Log level appropriate for event (ERROR for failures, INFO for milestones)
- [ ] Context includes relevant identifiers (run_id, user_id, thread_id)
- [ ] No sensitive data in logs

### Testing

- [ ] Unit tests for service logic
- [ ] Integration tests for changed contracts
- [ ] Test credentials via `settings.test.*`
- [ ] Regression tests for bug fixes

### Code Style

- [ ] Passes `ruff` linting
- [ ] Passes `basedpyright` type checking
- [ ] Follows naming conventions (snake_case for functions/variables)
- [ ] No broad `except Exception:` without logging/re-raising

---

## Linting & Type Checking

### Ruff (Linter)

**Run:**
```bash
uv run ruff check backend/src/
```

**Common fixes:**
- Remove unused imports
- Fix line length (max 100 chars)
- Use `async def` for async functions
- Add type annotations

### Basedpyright (Type Checker)

**Run:**
```bash
uv run basedpyright backend/src/
```

**Common fixes:**
- Add type hints for function parameters
- Use `Optional[T]` for optional values
- Fix `reportImplicitRelativeImport` warnings
- Handle `None` cases explicitly

### Pre-commit Hooks

**Setup:**
```bash
uv run pre-commit install
```

**Runs automatically on:**
- `ruff check`
- `basedpyright`
- Formatting checks

---

## Common Mistakes

### ❌ Missing Type Annotations

```python
# WRONG: No type hints
def get_user(user_id):
    return repository.get_by_id(user_id)
```

**Right:**
```python
async def get_user(user_id: UUID) -> User | None:
    return await repository.get_by_id(user_id)
```

### ❌ Catch-All Exception Handling

```python
# WRONG: Catching all exceptions without logging
except Exception:
    return None
```

**Right:**
```python
except SomeSpecificError as exc:
    logger.error("operation_failed", error=str(exc), exc_info=True)
    raise ApiProblemError(...) from exc
```

### ❌ Timezone-Naive Datetimes

```python
# WRONG: No timezone
created_at = datetime.now()
```

**Right:**
```python
created_at = datetime.now(timezone.utc)
```

### ❌ Testing with Hardcoded Credentials

```python
# WRONG: Hardcoded test DB
DATABASE_URL = "postgres://user:pass@localhost:5432/test"
```

**Right:**
```python
from core.config.settings import config

# Use settings.test.*
assert config.database.host is not None
```