/code-review

name: code-review
description: Review changed code against project standards. Checks for missing tests, dead code, type safety, lint issues, and coding conventions. Run after completing any implementation work.
user_invocable: true

Code Review

Review all changed code against the project's quality standards and coding conventions.

Code Standards

Read and internalize these standards before writing code. The review steps below verify compliance.

Supported interpreters

**CPython 3.11** is the baseline — what `docker/standalone/Dockerfile` ships, and what the `.python-version` pin and `uv.lock` resolve for. The package supports 3.11 through 3.14; `build-api-python-versions` in CI installs and smoke-tests each of them, so a dependency floor that excludes one of those interpreters is a break, not a follow-up.

Python Style

• Python 3.11+, type hints required — and see Supported interpreters above
• Async throughout (asyncpg, async FastAPI)
• Pydantic models for request/response
• Ruff for linting (line-length 120)
• No Python files at project root - maintain clean directory structure
• **Never use multi-item tuple return values** — not even for internal/private functions. Always use a dataclass or Pydantic model. No exceptions, no "it's just two values" shortcuts. If a function returns more than one value, define a named type for it.

Type Safety with Pydantic Models

**NEVER use raw `dict` types for structured data** — this applies to all code, including internal helpers and private functions. If the dict has known keys, it must be a dataclass or Pydantic model:

• Use Pydantic `BaseModel` for all data structures passed between functions
• Use `@dataclass` for lightweight internal data containers when Pydantic validation isn't needed
• Add `@field_validator` for type coercion (e.g., ensuring datetimes are timezone-aware)
• Avoid `dict.get()` patterns - use typed model attributes instead
• Parse external data (JSON, API responses) into Pydantic models at the boundary
• This catches type errors at parse time, not deep in business logic
• The only acceptable `dict` usage is for truly dynamic/unknown keys (e.g., arbitrary metadata, JSON blobs with no fixed schema)

# BAD - error-prone dict access
def process(data: dict) -> str:
    return data.get("name", "")  # No validation, silent failures

# GOOD - typed and validated
class UserData(BaseModel):
    name: str
    created_at: datetime

    @field_validator("created_at", mode="before")
    @classmethod
    def ensure_tz_aware(cls, v):
        if isinstance(v, str):
            v = datetime.fromisoformat(v.replace("Z", "+00:00"))
        if v.tzinfo is None:
            return v.replace(tzinfo=timezone.utc)
        return v

def process(data: UserData) -> str:
    return data.name  # Type-safe, validated at construction

TypeScript Style

• Next.js App Router for control plane
• Tailwind CSS with shadcn/ui components

Control plane on phones (390px wide)

`globals.css` already covers the page never scrolling sideways, hover-only controls showing on touch screens, and form fields not zooming on iOS. The checks below are the part a shared rule can't cover:

• Page sub-tabs use `components/ui/underline-tabs.tsx`, not a hand-rolled button row.
• Any row of filters, toggles or buttons wraps (`flex-wrap`) or scrolls inside itself

(`max-w-full overflow-x-auto` with `shrink-0 whitespace-nowrap` children).

• No fixed widths above ~340px without a phone fallback (`w-full sm:w-[420px]`,

`w-[min(420px,calc(100vw-1rem))]`). Side-by-side panes stack below `md`.

• Every table goes through `components/ui/table.tsx`: it scrolls inside its own frame

and keeps a 40rem floor so columns don't get squashed on a phone.

• Form sections laid out in a grid need `min-w-0` (the shared `Section` in

`form-layout.tsx` has it); side-by-side inputs stack below `sm`.

• Full-height layouts use `h-dvh`/`100dvh`, not `h-screen`/`100vh` (iOS hides the

bottom under the URL bar otherwise).

Code Comments

• **Always comment non-trivial technical decisions** with the reasoning behind the choice. If someone would ask "why is it done this way?", there should be a comment.
• **Keep comments up to date with history** — when changing an approach, update the comment to explain what was tried before and why it was changed. Comments serve as a tracker of previous implementations that likely had problems.
• Don't comment obvious code — only where the "why" isn't self-evident from the code itself.

# BAD - no context for future readers
results = await asyncio.gather(*tasks, return_exceptions=True)

# GOOD - explains the non-obvious choice
# Use return_exceptions=True to avoid cancelling sibling tasks on failure.
# Previously we used TaskGroup but it cancelled all tasks when one failed,
# causing partial writes that left orphaned entity links (see #412).
results = await asyncio.gather(*tasks, return_exceptions=True)

API Layer & Data Access

• **No direct database access in `api/http.py`** (or any API router). HTTP handlers must not build SQL, call `acquire_with_retry` / `conn.fetch` / `conn.fetchrow` / `conn.execute`, or reference `fq_table(...)`. All persistence and queries live in `MemoryEngine` (the engine layer). A handler parses/validates the request, calls an engine method, shapes the HTTP response, and maps domain results to status codes (e.g. a `None` return → 404).
• **Authentication/tenancy is enforced inside each engine method, not assumed by the handler.** Every engine method that touches bank-scoped data must authenticate via `request_context` — typically `await self._authenticate_tenant(request_context)` (often indirectly through `get_bank_profile(...)`) — so the correct tenant schema is resolved before any query runs. Handlers must thread `request_context` through to the engine method; never query a tenant-scoped table assuming the schema is already set.
• Engine methods return typed models (Pydantic/datac