mirror of
https://github.com/escalante29/WealthySmart.git
synced 2026-07-17 11:08:47 +02:00
212 lines
18 KiB
Markdown
212 lines
18 KiB
Markdown
# Architect Review — WealthySmart
|
||
|
||
> Hat: Staff-level Software Architect. Focus: structure, layering, data model, migration story, API consistency, deploy topology.
|
||
>
|
||
> **Editorial note:** ARCH-01 was corrected after verification — see the finding itself and `README.md`.
|
||
|
||
## Executive Summary
|
||
|
||
WealthySmart is a single-user personal finance management application with a modern full-stack architecture: FastAPI + SQLModel backend (PostgreSQL) paired with a React 19 + Vite frontend served through a Hono proxy, plus an LLM-powered AI agent interface. The system handles multi-currency transactions (CRC/USD/EUR/BTC/XMR), budget forecasting with complex billing-cycle logic, pension/municipal bill tracking, and push notifications. The architecture is sound overall — no fundamental redesign is needed — but there are weak defaults in configuration, zero test coverage, an ad-hoc migration story, and consistency gaps in validation and response schemas that should be addressed incrementally.
|
||
|
||
## How the System Fits Together
|
||
|
||
```
|
||
┌─────────────────────────────────────────────────────────────────┐
|
||
│ DEPLOYMENT (Docker Compose) │
|
||
├─────────────────────────────────────────────────────────────────┤
|
||
│ │
|
||
│ ┌──────────────────────┐ ┌─────────────────────┐ │
|
||
│ │ Frontend (Node) │ │ Backend (FastAPI) │ │
|
||
│ │ ┌─────────────────┐ │ │ ┌───────────────┐ │ │
|
||
│ │ │ Vite (port 3000)│ │ │ │ Uvicorn │ │ │
|
||
│ │ │ React 19 + TS │ │ │ │ (port 8000) │ │ │
|
||
│ │ └─────────────────┘ │ │ └───────────────┘ │ │
|
||
│ │ ┌─────────────────┐ │ │ ┌───────────────┐ │ │
|
||
│ │ │ Hono Server │ │ │ │ 15 endpoints │ │ │
|
||
│ │ │ (port 3001) │ │ │ │ + LLM agent │ │ │
|
||
│ │ │ • SPA serving │ │ │ └───────────────┘ │ │
|
||
│ │ │ • CopilotKit │ │ │ │ │
|
||
│ │ │ runtime │ │ │ │ │
|
||
│ │ │ • /api proxy │ │ │ │ │
|
||
│ │ └─────────────────┘ │ │ │ │
|
||
│ └──────────────────────┘ └─────────────────────┘ │
|
||
│ │ │ │
|
||
│ └────────────────────────────┘ │
|
||
│ │ │
|
||
│ ┌──────────────────────────────────┐ │
|
||
│ │ PostgreSQL 16 (port 5432) │ │
|
||
│ │ • 15 tables + enums │ │
|
||
│ │ • No Alembic migrations │ │
|
||
│ └──────────────────────────────────┘ │
|
||
│ │
|
||
│ Production: nginx-proxy + Let's Encrypt on single VPS │
|
||
│ Ingestion: n8n flows POST to /api/v1 with API tokens │
|
||
└─────────────────────────────────────────────────────────────────┘
|
||
```
|
||
|
||
**Key data flows:**
|
||
- Frontend requests `/api/v1/...` (proxied by Hono to FastAPI)
|
||
- Agent requests go to `/api/v1/agent/agui` (Microsoft Agent Framework + OpenAI)
|
||
- Exchange rates refreshed every 6 hours from multiple fallback APIs
|
||
- Auth via httpOnly cookies (SPA) or Bearer tokens (API clients / n8n)
|
||
|
||
## Strengths
|
||
|
||
1. **Modular endpoint structure** — 15 focused routers (accounts, transactions, budget, analytics, etc.) with clear single responsibilities.
|
||
2. **Sophisticated budget projection logic** — `budget_projection.py` correctly handles billing cycles (18th–18th for credit cards vs. 1st–1st calendar month), deferred transactions, recurring items with override amounts, and cumulative balance tracking across years, with a clean API (`compute_monthly_projection`, `compute_yearly_projection_with_cumulative`).
|
||
3. **Resilient exchange-rate handling** — fallback chain (BCCR → ExchangeRate-API → currency-api → FloatRates → CoinGecko) with in-memory and database caching; uses a `_last_known_*` pattern to retain previous values during API outages.
|
||
4. **Clean auth model** — dual-mode authentication (httpOnly cookie for SPA, Bearer token for API clients) with JWT + API-token support. Token revocation marks rows inactive rather than deleting them.
|
||
5. **Seed data for development** — comprehensive default categories, accounts, and recurring items; single-user design appropriate for the app.
|
||
6. **Type-safe models** — SQLModel schemas enforce some constraints (unique categories, year/month combinations) at the DB level.
|
||
7. **REST conventions mostly respected** — POSTs return 201, pagination via `offset`/`limit` is consistently applied.
|
||
8. **Consistent error surface** — `HTTPException` with clear status codes and detail messages throughout.
|
||
|
||
## Findings
|
||
|
||
### [ARCH-01] Weak secret hygiene around `.env` files and config defaults
|
||
**Severity:** High (downgraded from Critical after verification)
|
||
**Location:** `.env`, `.env.prod` (working tree), `backend/app/config.py:6–9`, `docker-compose.yml`
|
||
**Evidence (corrected):** `.env` and `.env.prod` exist in the repo root and contain real secrets (OpenAI API key, DB password, admin credentials, VAPID keys), but **they are gitignored and were never committed** — `git ls-files` and `git log --all --diff-filter=A` confirm no history. The genuine issues are: (a) `config.py` ships working fallback credentials (`SECRET_KEY="change-me-in-production"`, `ADMIN_USERNAME/PASSWORD = "admin"`), (b) `docker-compose.yml` hardcodes dev DB credentials, and (c) plaintext secrets on the dev machine with no rotation discipline.
|
||
**Why it matters:** If a deploy ever runs without its env file (typo in compose, CI regression — note `reference_gitea_deploy`: `.env.prod` is regenerated each deploy), the app silently boots with `admin/admin` and a known JWT signing key.
|
||
**Recommendation:** Remove the defaults and fail fast at startup (see SEC-01). Keep `.env.example` as the documented template. Rotate any key that has been pasted into chats/logs.
|
||
|
||
### [ARCH-02] No Formal Database Migration Strategy
|
||
**Severity:** High
|
||
**Location:** `backend/app/db.py:13–76`; no `alembic/` directory
|
||
**Evidence:** Migrations are ad-hoc SQL strings in `run_migrations()` with `IF NOT EXISTS` clauses. Adding a column requires hand-written `ALTER TABLE` code. Schema evolution is not version-controlled or reversible.
|
||
**Why it matters:** Manual migrations are error-prone and have no rollback. Dev and prod can diverge silently. The project rule "never reset prod DB; use migrations" makes this the single most important infrastructure gap.
|
||
**Recommendation:** Introduce Alembic: `alembic init`, autogenerate a baseline from current models, convert the existing manual statements, run `alembic upgrade head` at container startup. Effort: **M** (1–2 days).
|
||
|
||
### [ARCH-03] Validation & Error-Handling Inconsistencies
|
||
**Severity:** Medium
|
||
**Location:** `endpoints/transactions.py:71–90`, `endpoints/budget.py:108–109`, `endpoints/analytics.py:56`
|
||
**Evidence:** `list_transactions()` mixes `cycle_year/cycle_month` and `start_date/end_date` filters with `or_` in a nested query — unclear precedence. `budget.py` uses positional `HTTPException(400, ...)` while others use keyword args. `analytics.py:56` filters with the string literal `"COMPRA"` instead of `TransactionType.COMPRA`.
|
||
**Why it matters:** Mixing filter modes risks undefined behavior; hardcoded enum strings break refactoring safety; inconsistent error style hampers debugging.
|
||
**Recommendation:** Extract a `build_transaction_query()` helper with explicit precedence; standardize on `status_code=` kwargs; replace string literals with enum members. Effort: **S**.
|
||
|
||
### [ARCH-04] Agent Tools Duplicate Service Logic
|
||
**Severity:** Medium
|
||
**Location:** `backend/app/agent/tools.py`, `backend/app/services/budget_projection.py`
|
||
**Evidence:** `tools.py` re-implements queries (`get_accounts()`, `get_net_worth()`, `get_cycle_summary()`) that overlap with the service layer instead of delegating to it.
|
||
**Why it matters:** Budget logic changes must be applied in two places or the assistant and the UI disagree about the user's money.
|
||
**Recommendation:** Make agent tools thin wrappers over service functions. Effort: **M** (1 day).
|
||
|
||
### [ARCH-05] No Input Limits on Bulk Operations
|
||
**Severity:** Medium
|
||
**Location:** `endpoints/import_transactions.py:96–149`
|
||
**Evidence:** `paste_import()` accepts arbitrary text with no size cap; a 10 MB paste is parsed entirely in memory before any validation.
|
||
**Why it matters:** Memory-exhaustion DoS vector; bulk operations should validate request size upfront.
|
||
**Recommendation:** `Field(..., max_length=1_000_000)` on the request model; pre-validate before opening the write loop. Effort: **S**.
|
||
|
||
### [ARCH-06] Missing Test Coverage
|
||
**Severity:** High
|
||
**Location:** No `tests/` directory anywhere; `requirements.txt` lacks pytest
|
||
**Evidence:** Zero test files in the repository.
|
||
**Why it matters:** Budget projection is the most intricate logic in the app (billing-cycle edges, deferred carryover, cumulative balances with overrides) and is entirely unguarded against regression.
|
||
**Recommendation:** `backend/tests/` with `test_budget_projection.py` (Feb 28/29, year boundaries, overrides), `test_auth.py`, `test_exchange_rate.py`; Vitest for `useBudget`. Effort: **L** (3–5 days for meaningful coverage).
|
||
|
||
### [ARCH-07] Single Hono Instance Is a Single Point of Failure
|
||
**Severity:** Low (revised for a single-user app)
|
||
**Location:** `frontend/server.ts:1–20`, `frontend/Dockerfile`, `docker-compose.prod.yml`
|
||
**Evidence:** One Hono container serves SPA + CopilotKit runtime + proxy. The Dockerfile healthcheck probes the backend's `/api/health`, not Hono's own readiness.
|
||
**Why it matters:** If Hono crashes the whole app is down even with a healthy backend. For a single-user app, replicas are over-engineering; a correct healthcheck + restart policy is the right-sized fix.
|
||
**Recommendation:** Healthcheck Hono itself; `restart: unless-stopped`; add 5xx retry in the frontend API client. Skip load balancing.
|
||
|
||
### [ARCH-08] Exchange-Rate Refresh Loop Swallows Cancellation
|
||
**Severity:** Low
|
||
**Location:** `backend/app/services/exchange_rate.py:348–366`, `backend/app/main.py:66`
|
||
**Evidence:** `refresh_rates_periodically()` runs `while True` with a broad except that logs and continues; cancellation during shutdown can be silently swallowed.
|
||
**Recommendation:** Catch `asyncio.CancelledError` explicitly and `break`; keep the broad handler for everything else. Effort: **S**.
|
||
|
||
### [ARCH-09] No Foreign-Key Delete Rules
|
||
**Severity:** Medium
|
||
**Location:** `backend/app/models/models.py:144, 258, 427`
|
||
**Evidence:** `Transaction.category_id` and similar FKs declare `foreign_key=` with no `ondelete` behavior; deleting a category leaves dangling references.
|
||
**Why it matters:** Silent referential inconsistency — budget rows pointing at categories that no longer exist.
|
||
**Recommendation:** Decide per-FK between `SET NULL` (transactions should outlive categories) and `CASCADE` (water readings die with their receipt); apply via Alembic migration. Effort: **S**.
|
||
|
||
### [ARCH-10] Fragile Cookie Parsing in Agent Middleware
|
||
**Severity:** Medium
|
||
**Location:** `backend/app/main.py:88–142`
|
||
**Evidence:** Cookie extraction uses regex `r"(?:^|;\s*)ws_token=([^;]+)"`, which mishandles edge cases (multiple cookies of the same name, unusual encodings).
|
||
**Recommendation:** Use stdlib `http.cookies.SimpleCookie`. Effort: **S**.
|
||
|
||
### [ARCH-11] `deferred_to_next_cycle` Implemented in Backend but Not Exposed in UI
|
||
**Severity:** Medium
|
||
**Location:** `backend/app/models/models.py:145`, `endpoints/transactions.py:219–235`
|
||
**Evidence:** The field is settable via PATCH and respected by budget projection; the deferral toggle exists in row actions but `TransactionModal.tsx` has no field for it, and the affordance is undiscoverable (see UX-05).
|
||
**Recommendation:** Add the checkbox to `TransactionModal.tsx` and make the row-action state visible. Effort: **S**.
|
||
|
||
### [ARCH-12] Pagination Lacks a Deterministic Tiebreaker
|
||
**Severity:** Low
|
||
**Location:** `endpoints/transactions.py:96`, `endpoints/salarios.py:35–36`
|
||
**Evidence:** Paginated queries order by `date DESC` only; equal dates yield non-deterministic page boundaries.
|
||
**Recommendation:** `.order_by(col(...).date.desc(), col(...).id.desc())` everywhere paginated. Effort: **S**.
|
||
|
||
### [ARCH-13] Mutation → Refetch-Everything Pattern
|
||
**Severity:** Low
|
||
**Location:** `frontend/src/hooks/useBudget.ts:61–74`
|
||
**Evidence:** Every mutation triggers `Promise.all([fetchProjection(), fetchMonthDetail(), fetchRecurringItems()])` regardless of what changed.
|
||
**Why it matters:** Wasteful, and the pattern will multiply as modules grow. Acceptable as a simplicity tradeoff today; the durable fix is a query library (see FE-10 / Plan Phase 3).
|
||
|
||
### [ARCH-14] Hardcoded Dev DB Credentials in Compose
|
||
**Severity:** Medium
|
||
**Location:** `docker-compose.yml` (db service `environment:` block)
|
||
**Evidence:** `POSTGRES_PASSWORD: wealthy_pass` hardcoded in source control (prod compose correctly uses `${POSTGRES_PASSWORD}`).
|
||
**Recommendation:** Move dev credentials to the gitignored `.env`; reference via `${...}` in both compose files. Effort: **S**.
|
||
|
||
### [ARCH-15] N+1 Query in Category Aggregation
|
||
**Severity:** Medium
|
||
**Location:** `backend/app/services/budget_projection.py:370–383`
|
||
**Evidence:** `compute_cc_by_category()` calls `session.get(Category, cat_id)` inside a loop — one query per category with spending.
|
||
**Recommendation:** Prefetch `{c.id: c for c in session.exec(select(Category)).all()}` and look up in the dict. Effort: **S** (minutes).
|
||
|
||
### [ARCH-16] API Response Schema Inconsistency
|
||
**Severity:** Low
|
||
**Location:** `endpoints/transactions.py:28–33` (Pydantic models) vs `endpoints/budget.py:100–157` (raw dicts) vs `endpoints/analytics.py:18–23`
|
||
**Evidence:** Some endpoints return typed models, others plain dicts; OpenAPI schema generation is incomplete and frontend types are maintained by hand.
|
||
**Recommendation:** Shared models in `app/api/schemas.py` + `response_model=` on every route; this also unlocks frontend type codegen (see FE-14). Effort: **M**.
|
||
|
||
### [ARCH-17] Agent Session via ContextVar Couples Tools to HTTP Middleware
|
||
**Severity:** Low
|
||
**Location:** `backend/app/agent/tools.py:40–52`, `backend/app/main.py:131–141`
|
||
**Evidence:** Tools fetch the DB session from a `contextvars.ContextVar` set by middleware; calling a tool outside a request raises `LookupError`.
|
||
**Recommendation:** Document the constraint and add a clear error message (see BE-12); full refactor only if tools ever need to run outside HTTP. Effort: **S** for the guard.
|
||
|
||
### [ARCH-18] No Data Export / Archival Path
|
||
**Severity:** Low
|
||
**Location:** All tables are append-only with no export endpoint
|
||
**Why it matters:** Acceptable growth for a personal app, but there is no way to get historical data out (taxes, accountant, backup verification).
|
||
**Recommendation:** CSV export endpoint per major table (also UX wishlist #5). Effort: **M**.
|
||
|
||
### [ARCH-19] Hono Proxy Trusts Upstream Blindly
|
||
**Severity:** Low
|
||
**Location:** `frontend/server.ts` proxy handlers
|
||
**Evidence:** Responses are piped through without shape validation; combined with FE-14 (no runtime validation client-side), schema drift fails confusingly.
|
||
**Recommendation:** Don't validate in the proxy (wrong layer); fix via shared response schemas (ARCH-16) and optional Zod at the API-client boundary.
|
||
|
||
### [ARCH-20] Lazy Imports Papering Over a Circular Dependency
|
||
**Severity:** Low
|
||
**Location:** `backend/app/auth.py:39–43`
|
||
**Evidence:** `_validate_token()` imports `get_session` and `APIToken` inside the function body to dodge a cycle with `db.py`.
|
||
**Recommendation:** Split token utilities into their own module to break the cycle properly. Effort: **S**.
|
||
|
||
## Recommended Refactors (prioritized)
|
||
|
||
| # | Item | Effort | Phase |
|
||
|---|---|---|---|
|
||
| 1 | ARCH-01/SEC-01: fail-fast on default secrets, fix dev compose creds | S | 0 |
|
||
| 2 | ARCH-02: Alembic migrations | M | 1 |
|
||
| 3 | ARCH-06: test suite (budget projection first) | L | 1 |
|
||
| 4 | ARCH-15: N+1 fix in `compute_cc_by_category` | S | 2 |
|
||
| 5 | ARCH-03: standardize validation/error handling/enums | S | 2 |
|
||
| 6 | ARCH-09: FK delete rules via migration | S | 2 |
|
||
| 7 | ARCH-04: agent tools delegate to services | M | 2 |
|
||
| 8 | ARCH-16: shared response schemas (`response_model=` everywhere) | M | 3 |
|
||
| 9 | ARCH-13/FE-10: query library on the frontend | M | 3 |
|
||
| 10 | ARCH-11: expose deferred-transaction UI | S | 4 |
|
||
| 11 | ARCH-18: CSV export | M | 4 |
|
||
| 12 | ARCH-05/08/10/12/14/17/20: small hardening fixes | S each | 0–2 |
|
||
|
||
**Overall assessment:** Production-ready for a single-user personal app, with the caveat that configuration defaults must fail fast before anything else. The architecture needs no redesign — invest in migrations, tests, and consistency.
|