WealthySmart/codebase-review/02-backend.md

# Backend Review — WealthySmart

> Hat: Senior Backend Engineer (Python / FastAPI / SQLModel). Focus: correctness, money & date math, query efficiency, PDF parsing, agent tools, migrations, tests.

## Executive Summary

The WealthySmart backend is a well-structured FastAPI application with a PostgreSQL database and SQLModel ORM. The codebase demonstrates solid architectural patterns (dependency injection, a real service layer, API tokenization) but contains several correctness and robustness gaps that deserve attention: timezone-naive `datetime.utcnow()` everywhere, `float` for all monetary values, a suspected off-by-one in billing-cycle math, N+1 query patterns in the agent tools, non-transactional multi-file PDF uploads, blocking subprocess calls inside `async` routes, and broad exception handlers that can mask failures. The codebase has no tests at all, which compounds every other risk — especially around the budget-cycle logic.

## Strengths

1. **Clean architecture** — well-organized service layer (`exchange_rate.py`, `budget_projection.py`), clear separation with one endpoint file per domain.
2. **Robust exchange-rate handling** — multi-source fallback (BCCR → ExchangeRate-API → CoinGecko) with in-memory caching and last-known fallbacks.
3. **Flexible budget system** — projection engine handles recurring items, 18th–18th billing cycles, and deferred transactions with carryover logic.
4. **Security practices** — SHA-256 token hashing, OAuth2 bearer support, dual auth (JWT + API tokens), credentials hidden from logs.
5. **API token management** — secure generation, expiration support, hash-based lookup.
6. **Graceful degradation** — push-notification failures don't crash the request path; stale rates keep the app usable during API outages.
7. **Comprehensive PDF services** — municipal-receipt and pension parsing with layered regex extraction.

## Findings

### [BE-01] `datetime.utcnow()` — Deprecated and Timezone-Naive — High
**Location:** `auth.py:19`, `auth.py:51`, `budget.py:262`, `services/exchange_rate.py:139+`, 10+ call sites
**Evidence:** `datetime.utcnow()` used throughout; deprecated since Python 3.12 in favor of `datetime.now(timezone.utc)`.
**Why it matters:** Mixing naive and aware datetimes causes silent comparison bugs, and Postgres timestamp semantics depend on consistency. Costa Rica is UTC-6 year-round — naive "UTC" timestamps compared against local dates shift transactions across cycle boundaries near midnight.
**Recommendation:** Mechanical replacement with `datetime.now(timezone.utc)`; audit each comparison site for naive/aware mixing; prefer `TIMESTAMPTZ` columns going forward.

### [BE-02] Float Instead of Decimal for Money — High
**Location:** `models/models.py:102, 104, 131, 184+` (all financial fields)
**Evidence:** `amount: float`, `balance: float`, `buy_rate: float`, `saldo_final: float` across the schema.
**Why it matters:** Binary floats can't represent most decimal amounts exactly; cumulative budget math, projections, and historical audits accumulate error. Industry standard for money is `Decimal`/`NUMERIC`.
**Recommendation:** Migrate monetary fields to `Decimal` (`Field(max_digits=15, decimal_places=2)` → Postgres `NUMERIC`); update `budget_projection.py` and `exchange_rate.py` arithmetic. Do this *after* Alembic exists (one clean migration) and *with* tests in place.

### [BE-03] Savings Accrual Lacks Atomicity — Medium
**Location:** `services/savings_accrual.py:60`
**Evidence:** `session.commit()` with no rollback path; if MEMP or MPAT accounts are missing, the function returns early with partial state possible.
**Recommendation:** Raise early if required accounts are absent; wrap the mutation block so it commits entirely or not at all.

### [BE-04] Pension Snapshot Dedup Done in Python — Medium
**Location:** `agent/tools.py:269–278`
**Evidence:** Loads all `PensionSnapshot` rows, then deduplicates by fund in a Python loop.
**Recommendation:** `SELECT DISTINCT ON (fund) ... ORDER BY fund, period_end DESC` (PostgreSQL) or a window-function query.

### [BE-05] N+1 in Municipal Receipts Water Readings — Medium
**Location:** `agent/tools.py:320–344`
**Evidence:** Loads receipts (line 324), then queries `WaterMeterReading` per receipt inside the loop (line 328) — 13 queries where 1–2 would do.
**Recommendation:** Eager-load via `selectinload` or fetch all readings for the receipt IDs in one `IN (...)` query.

### [BE-06] Possible Off-by-One in Billing-Cycle Calculation — High
**Location:** `services/budget_projection.py:78–85, 103–108`
**Evidence:** `get_cycle_range(year, month)` returns `(month/18, month+1/18)` while the comment says "cycle (M-1): from (M-1)/18 to M/18, paid with month M salary" — comment and code disagree about which cycle a month's budget uses.
**Why it matters:** If wrong, every monthly budget is aligned to the wrong statement period — salary and expenses misreported by a month. If right, the comment is wrong and the next maintainer will "fix" working code.
**Recommendation:** This is the **first thing to pin down with tests**: assert which cycle a Feb-15 transaction lands in for the March budget against known-good real data, then fix code or comment accordingly.

### [BE-07] No Month/Year Validation in Date Helpers — Medium
**Location:** `services/budget_projection.py:73–85`
**Evidence:** `datetime(year, month, 18)` with no range check; month 13 raises an unhandled `ValueError` from deep inside projection math.
**Recommendation:** Validate `1 <= month <= 12` and a sane year range at the helper boundary with a clear error message.

### [BE-08] Duplicate Detection Is Incomplete — Medium
**Location:** `endpoints/transactions.py:182–191`, `import_transactions.py:118–128`
**Evidence:** Dedup keys off the nullable `reference` field; manual transactions and hash collisions/misses can create duplicates that skew analytics.
**Recommendation:** Tighten the reference-hash inputs (include `card_last4`), and consider a unique constraint on `(date, merchant, amount, currency, source)` with an explicit override path for genuine same-day repeat purchases.

### [BE-09] PDF Temp Files Not Cleaned on Timeout — Medium
**Location:** `services/pension_pdf.py:46–50`, `services/municipal_receipt_pdf.py:79`
**Evidence:** `subprocess.run(..., timeout=30/10)` is good, but on timeout/exception the `NamedTemporaryFile` cleanup path isn't guaranteed.
**Recommendation:** Use `tempfile.TemporaryDirectory()` as a context manager so cleanup is unconditional.

### [BE-10] Multi-File Uploads Are Not Transactional — High
**Location:** `endpoints/pensions.py:104–162`, `municipal_receipts.py:197–228`
**Evidence:** The loop commits per file; if file 2 fails after file 1 committed, the batch is half-applied with no undo.
**Why it matters:** Partial uploads create inconsistent state the user can't easily see or revert.
**Recommendation:** Either wrap the batch in one transaction (all-or-nothing) or keep per-file commits but return a precise per-file success/failure manifest (the response models already have `errors[]` — make the frontend show it, see UX-09).

### [BE-11] Nullable FKs Without Delete Behavior — Medium
**Location:** `models/models.py:144` (Transaction.category_id), `:258` (RecurringItem.category_id)
**Evidence:** Optional FKs with no `ondelete` rule; deleting a category strands references.
**Recommendation:** Same as ARCH-09 — explicit `SET NULL`/`CASCADE` per relationship, applied via migration.

### [BE-12] Agent Session ContextVar Failure Is Cryptic — Medium
**Location:** `agent/tools.py:51–52`
**Evidence:** `_session_ctx.get()` raises bare `LookupError` if middleware didn't bind a session.
**Recommendation:** Catch and re-raise as `RuntimeError("DB session not bound to agent context — tool called outside request scope")`.

### [BE-13] Locale-Naive Amount Parsing in Paste Import — Low
**Location:** `endpoints/import_transactions.py:77`
**Evidence:** `match.group(1).replace(",", "")` assumes `,` is always a thousands separator.
**Recommendation:** Document the assumed format (BAC statement format) in the docstring; add a sanity check (e.g., reject if the result is 100× off from a `.` interpretation).

### [BE-14] Blocking Subprocess Inside Async Routes — Medium
**Location:** `endpoints/pensions.py:105–162`, `municipal_receipts.py:197–228`
**Evidence:** `async def upload_...()` calls `parse_pension_pdf()` which runs `subprocess.run()` synchronously — blocks the event loop for up to 30s per file.
**Recommendation:** `await asyncio.to_thread(parse_pension_pdf, pdf_bytes)` (the codebase already uses `asyncio.to_thread` for rate refresh, so the pattern exists).

### [BE-15] Hardcoded Default Credentials — High
**Location:** `config.py:6, 8–9`
**Evidence:** `SECRET_KEY: str = "change-me-in-production"`, `ADMIN_USERNAME: str = "admin"`, `ADMIN_PASSWORD: str = "admin"` (verified).
**Why it matters:** A deploy that loses its env file boots wide open. See SEC-01 for the exploit framing.
**Recommendation:** Remove defaults; raise at startup if unset or still default.

### [BE-16] Agent Budget Tool Has No Year Bounds — Low
**Location:** `agent/tools.py:212` (vs `endpoints/budget.py:131` which checks MIN_YEAR/MAX_YEAR)
**Evidence:** REST endpoint validates year range; the agent tool calling the same projection logic does not — year 9999 reaches `compute_yearly_projection_with_cumulative()`.
**Recommendation:** Apply the same MIN/MAX_YEAR bounds in the tool.

### [BE-17] Agent Tools Have Fixed Limits, No Pagination — Low
**Location:** `agent/tools.py:269, 327–344`
**Evidence:** Hardcoded `limit=12` / `limit=50` with no offset parameter.
**Recommendation:** Add optional `offset`/`limit` tool parameters so the assistant can answer questions about older history.

### [BE-18] `except Exception: pass` Swallows Errors — Low
**Location:** `services/exchange_rate.py:63–64, 93–94, 108–109, 122–123`
**Evidence:** Bare pass on all exceptions in each rate-source fetcher.
**Why it matters:** Network failures, schema changes in upstream APIs, and genuine bugs all fail identically and invisibly.
**Recommendation:** Catch specific exceptions (`httpx.TimeoutException`, `json.JSONDecodeError`, `KeyError`) and `logger.warning(...)` per source so a dead source is observable.

### [BE-19] Ad-hoc Migrations Without Atomicity — Medium
**Location:** `db.py:13–77`
**Evidence:** Sequential `ALTER TABLE` strings with per-statement try/except/rollback; a mid-sequence failure leaves a partially migrated schema with no version record.
**Recommendation:** Alembic (duplicate of ARCH-02; listed here because the per-statement rollback pattern is itself a hazard).

### [BE-20] Stale-Rate Cache Has No Timestamp — Low
**Location:** `services/exchange_rate.py:31–37, 136–142`
**Evidence:** `_last_known` caches carry no `last_updated`; consumers can't tell a fresh rate from a week-old one.
**Recommendation:** Store a timestamp with the cached value and expose freshness (`{"rate": X, "as_of": ...}`) — the agent and UI can then warn on stale data (pairs with UX data-trust concerns).

### [BE-21] Projection Runs ~15+ Queries Per Call — Medium
**Location:** `services/budget_projection.py:114–222`
**Evidence:** For each `TransactionSource`, 4–6 separate aggregate queries; 3 sources ≈ 15+ round-trips per month, ×12 for the yearly view.
**Recommendation:** Collapse into one or two `GROUP BY source, transaction_type` aggregate queries.

### [BE-22] PensionSnapshot Upsert Has a Race Window — Low
**Location:** `models/models.py:329–335`
**Evidence:** `UniqueConstraint("fund", "period_start", "period_end")` exists, but the upsert is an application-level check-then-insert; concurrent uploads of the same PDF can collide.
**Recommendation:** Use `INSERT ... ON CONFLICT DO UPDATE` (`sqlalchemy.dialects.postgresql.insert`). Low priority — n8n is the only concurrent writer and runs daily.

### [BE-23] Account Labels Not Unique — Low
**Location:** `models/models.py:98–110`
**Evidence:** `label: str` without a unique constraint; two accounts named "BAC" are allowed.
**Recommendation:** Unique on `(bank, currency, label)` or at least surface duplicates in the UI.

### [BE-24] Hardcoded Fallback Exchange Rates in Agent — Low
**Location:** `agent/tools.py:77–99`
**Evidence:** `sell = rate.sell_rate if rate else 600.0` and a magic `1.08` EUR multiplier in `get_net_worth`.
**Why it matters:** The assistant reports a wrong net worth when the rate service is down — silently.
**Recommendation:** Fall back to the latest DB-persisted rate; if none, say so in the tool output instead of inventing a number.

### [BE-25] No Tests — Critical
**Location:** `backend/` (no `tests/`, no pytest in `requirements.txt`)
**Evidence:** Zero coverage on budget cycles, deferred logic, PDF parsing, auth.
**Recommendation:** pytest + fixtures (in-memory SQLite or a test Postgres). Minimum first targets: cycle boundaries (resolves BE-06), deferred carryover, PDF parser golden files, token auth paths.

## Quick Wins (under 1 hour each)

1. Startup check rejecting default `SECRET_KEY`/admin creds (`config.py`) — BE-15
2. `s/datetime.utcnow()/datetime.now(timezone.utc)/` sweep — BE-01 (verify comparisons after)
3. Month/year bounds in `budget_projection.py` helpers — BE-07
4. `DISTINCT ON` for pension snapshots — BE-04
5. Eager-load water readings — BE-05
6. `TemporaryDirectory()` in both PDF parsers — BE-09
7. Year bounds in agent budget tool — BE-16
8. Specific exceptions + logging in `exchange_rate.py` — BE-18
9. `asyncio.to_thread` around PDF parsing — BE-14
10. Clear error message for unbound agent session — BE-12

**Recommended sequence:** tests for cycle math first (BE-25 → BE-06), then the Decimal migration (BE-02) once Alembic (BE-19/ARCH-02) exists to carry it.