WealthySmart/codebase-review/06-weak-spots-inventory.md

# Weak-Spots Inventory — Consolidated Across All Five Hats

90+ raw findings from the five reviews, deduplicated into **13 themes**. Each theme lists the contributing finding IDs (traceable back to the hat reports), a consolidated severity, and the fix destination in [07-improvement-plan.md](07-improvement-plan.md).

Severity reflects synthesis judgment for a *single-user, publicly exposed personal finance app* — some agent severities were adjusted (noted inline).

---

## Severity Matrix

| # | Theme | Severity | Findings | Plan Phase |
|---|---|---|---|---|
| T1 | Auth & config hardening | **Critical** | SEC-01..05, SEC-08, BE-15, ARCH-01, ARCH-14 | 0 |
| T2 | Zero test coverage | **Critical** | ARCH-06, BE-25 | 1 |
| T3 | No real migration system | **High** | ARCH-02, BE-19 | 1 |
| T4 | Billing-cycle math unverified | **High** | BE-06, BE-07 | 1 |
| T5 | Money stored as float | **High** | BE-02 | 1–2 |
| T6 | Silent failures & missing feedback (frontend) | **High** | FE-02, FE-06, FE-11, UX-01, UX-02, UX-05, UX-09, UX-15 | 3 |
| T7 | Hand-rolled data fetching (races, no cache/cancel/timeout) | **High** | FE-01, FE-03, FE-09, FE-10, FE-18, FE-22, FE-24, ARCH-13 | 3 |
| T8 | Ingestion trust gap (silent n8n failures, opaque dedup) | **High** | UX-17, UX-20, BE-08 | 4 |
| T9 | Upload robustness | **Medium** | SEC-06, BE-09, BE-10, BE-14, FE-05, FE-19 | 2 |
| T10 | Datetime & timezone hygiene | **Medium** | BE-01 | 1 |
| T11 | Query efficiency & data-integrity constraints | **Medium** | ARCH-09, ARCH-12, ARCH-15, BE-03, BE-04, BE-05, BE-11, BE-21, BE-22, BE-23 | 2 |
| T12 | Agent quality & safety | **Medium** | ARCH-04, ARCH-17, BE-12, BE-16, BE-17, BE-24, SEC-09, UX-16, UX-23 | 2/4 |
| T13 | Consistency & polish (l10n, a11y, mobile, UX) | **Medium** | FE-07, FE-16, FE-17, UX-03, UX-04, UX-06, UX-07, UX-10..14, UX-18, UX-19, UX-21..25, ARCH-11 | 4 |

Standalone small items: FE-08/SEC-07 (security headers → Phase 0), FE-20 (dead code → Phase 0 cleanup), FE-23 (proxy 502 → Phase 0), ARCH-05 (paste size cap → Phase 0), ARCH-16/FE-14 (response schemas/codegen → Phase 3), ARCH-18 (CSV export → Phase 4), ARCH-08/10/20 (small backend hardening → Phase 2).

---

## T1 — Auth & Config Hardening · Critical

**The single most important theme.** The app is publicly exposed at wealth.cescalante.dev with:
- Working fallback credentials in `config.py` (`admin/admin`, known `SECRET_KEY`) that activate silently if a deploy loses its env file — and `.env.prod` is regenerated every deploy (SEC-01, BE-15)
- No rate limiting on login + non-constant-time comparison, against a fixed username (SEC-02, SEC-03)
- CORS reflecting any origin with credentials (SEC-04) — currently blunted only by SameSite=Lax
- 30-day irrevocable JWTs (SEC-05); `secure=False` cookie relying on a comment (SEC-08)
- Hardcoded dev DB credentials in `docker-compose.yml` (ARCH-14)

**Correction:** the "secrets committed to git" claim (original ARCH-01) was verified **false** — `.env*` is gitignored with no history. The theme remains Critical because of the fallback-credential failure mode, not repo leakage.

**Why Critical:** every item is individually small, but together they describe a public financial app one pipeline hiccup away from `admin:admin`. Total fix effort: about half a day.

## T2 — Zero Test Coverage · Critical

Not one test file in the repository; pytest isn't even a dependency. The riskiest logic in the app — billing-cycle boundaries, deferred-transaction carryover, cumulative balances with manual overrides, PDF regex parsing — is guarded by nothing. Every other theme's fixes (Decimal migration, cycle verification, refactors) are unsafe to attempt without this. **Tests are the enabling investment for the entire plan.**

## T3 — No Real Migration System · High

Schema evolution is hand-written `ALTER TABLE` strings in `db.py:13–77` with per-statement try/except. The project's own hard rule — *never reset prod DB* (`feedback_db_reset`) — makes versioned, reversible migrations non-optional. Alembic is the obvious choice; it also unblocks T5 (Decimal) and T11 (constraints), which both need schema changes.

## T4 — Billing-Cycle Math Unverified · High

`get_cycle_range()` code and its own comment disagree about which 18th–18th window a month's budget covers (BE-06). Either every monthly budget is shifted by one cycle, or the comment will mislead the next change into breaking working code. Helpers also accept month 13 without complaint (BE-07). **Resolve with characterization tests against known-real statement data — first deliverable of the test suite.**

## T5 — Money Stored as Float · High

All monetary columns are `float` (BE-02). For an app whose entire purpose is financial accuracy, `NUMERIC`/`Decimal` is the standard. Requires: Alembic (T3) first, tests (T2) to prove equivalence, then a coordinated model + service-math migration. Highest-effort single item in the backend; schedule deliberately, not as a quick fix.

## T6 — Silent Failures & Missing Feedback · High

The frontend's pervasive pattern: errors → `console.error`, success → nothing. Analytics renders empty charts on API failure (FE-02); deletes/edits/uploads complete without confirmation (UX-01); upload error details returned by the backend are never shown (UX-09); CopilotKit middleware eats its own exceptions (FE-11); destructive confirmation is inconsistent (UX-15). One toast system + one standard error/retry pattern resolves the lot.

## T7 — Hand-Rolled Data Fetching · High

No cancellation (FE-01, FE-03), no caching/dedup (FE-10), no timeouts (FE-24), refetch-everything-on-mutation (ARCH-13), 401 redirect loop risk (FE-22), coupled effects (FE-09). **Root cause is singular: there is no query layer.** Adopting TanStack Query retires the entire theme and makes T6's error/loading states near-free. This is the highest-leverage frontend decision available.

## T8 — Ingestion Trust Gap · High

The app's core promise is automatic ingestion, but: n8n parse failures are invisible inside the app (UX-17); dedup decisions are reported as a bare count with no review or override (UX-20); the dedup key itself has gaps (nullable `reference`, BE-08). A user discovers missing weeks of data only at month-end. Fix in two stages: a derived Sync Status page (cheap — `max(created_at)` per source + cadence thresholds), then an import-review flow with dedup detail.

## T9 — Upload Robustness · Medium

Unbounded reads with no type check (SEC-06), blocking `subprocess.run` inside async routes (BE-14), temp files leaking on timeout (BE-09), per-file commits leaving half-applied batches (BE-10), frontend hanging on post-upload refetch (FE-05). One focused pass over the two upload endpoints + their UI closes all of it.

## T10 — Datetime & Timezone Hygiene · Medium

`datetime.utcnow()` (deprecated, naive) at 10+ sites (BE-01). Costa Rica is UTC-6; naive-UTC timestamps compared against local dates can shift late-evening transactions across cycle boundaries. Mechanical fix + a comparison audit; do it alongside T4 so the tests catch any behavior change.

## T11 — Query Efficiency & Data-Integrity Constraints · Medium

N+1s (ARCH-15, BE-04, BE-05), ~15 queries per projection call (BE-21), FKs without delete rules (ARCH-09/BE-11), missing tiebreaker ordering (ARCH-12), check-then-insert race (BE-22), non-unique account labels (BE-23), non-atomic savings accrual (BE-03). None urgent for single-user load; all cheap; constraints need Alembic (T3) first.

## T12 — Agent Quality & Safety · Medium

Tools duplicate service logic so UI and assistant can drift (ARCH-04); hardcoded fallback rates make it confidently wrong when the rate service is down (BE-24); no year bounds (BE-16); cryptic failure when called out of context (BE-12); fixed limits hide older history (BE-17). Safety posture is acceptable *because tools are read-only* — keep that as an explicit invariant (SEC-09). UX: cards are ephemeral and nothing is exportable (UX-16, UX-23).

## T13 — Consistency & Polish · Medium

The long tail that makes the app feel finished: three month-navigation patterns (UX-03), three month-name implementations in two languages (UX-19, FE-17, UX-07), inert search on cash tab (UX-04), no-undo deletes (UX-10), dense mobile tables (UX-11), hardcoded pension assumptions (UX-12), cycle filter applying to one of three charts (UX-13), dead-end empty states (UX-14), deferred-transaction UI gap (ARCH-11, UX-05), a11y gaps on toggles (FE-16), meter-ID legends (UX-24). Individually small; batch them into themed polish passes.

---

## Cross-Hat Agreement (highest-confidence signals)

Where independent hats converged on the same problem, confidence is highest:

| Problem | Flagged independently by |
|---|---|
| Default credentials / weak config | Architect, Backend, Cybersec |
| No tests | Architect, Backend |
| No migrations | Architect, Backend |
| Upload validation/robustness | Backend, Cybersec, Frontend, Poweruser |
| Silent error swallowing | Frontend, Poweruser, Backend (exchange-rate `pass`) |
| Agent/service logic duplication | Architect, Backend |
| Localization inconsistency | Frontend, Poweruser |
| Deferred-transaction UI gap | Architect, Poweruser |
| Refetch-everything / no query layer | Architect, Frontend |