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.