Files
WealthySmart/codebase-review/README.md
Carlos Escalante 5c265129e8 Add five-hat codebase review and phased improvement plan
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-06-09 19:13:19 -06:00

46 lines
4.0 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# WealthySmart — Five-Hat Codebase Review
**Date:** 2026-06-09
**Scope:** Full repository (`backend/`, `frontend/`, compose files, CI workflow). `tech_docs/`, `node_modules/`, and `.claude/` excluded.
**Method:** Five independent senior/staff-level review agents, each with a distinct lens, followed by cross-verification of the highest-severity claims against the actual repo state and a consolidated synthesis.
## The Five Hats
| Hat | Report | Focus |
|---|---|---|
| 🏛 Architect | [01-architect.md](01-architect.md) | System structure, layering, data model, migrations, deploy topology |
| ⚙️ Backend | [02-backend.md](02-backend.md) | FastAPI/SQLModel correctness, money/date math, queries, PDF parsing, agent tools |
| 🎨 Frontend | [03-frontend.md](03-frontend.md) | React data fetching, state, type safety, a11y, Hono server, CopilotKit |
| 🔐 Cybersec | [04-security.md](04-security.md) | AuthN/Z, secrets, CORS, rate limiting, uploads, injection surface |
| 👤 Poweruser | [05-poweruser.md](05-poweruser.md) | Daily-use UX, feedback loops, data trust, consistency, feature gaps |
## Synthesis Documents
- **[06-weak-spots-inventory.md](06-weak-spots-inventory.md)** — All 90+ findings deduplicated into 13 cross-cutting themes with a severity matrix.
- **[07-improvement-plan.md](07-improvement-plan.md)** — Phased implementation plan (Phase 04) with concrete steps, acceptance criteria, and effort estimates.
## Editorial Corrections (verified against the repo)
Agent findings were spot-checked. One material correction:
- **`.env` / `.env.prod` are NOT committed to git.** Both are covered by `.gitignore` (`.env`, `.env.*`) and `git log --all --diff-filter=A -- .env .env.prod` returns nothing. The Architect's ARCH-01 ("secrets in version control", Critical) is **downgraded**: the files exist only in the working tree, which is expected. The *real* residual risks are (a) hardcoded fallback credentials in `backend/app/config.py` (verified: `SECRET_KEY = "change-me-in-production"`, `ADMIN_PASSWORD = "admin"`) and (b) hardcoded dev DB credentials in `docker-compose.yml`. These are tracked as SEC-01 / BE-15.
Claims that were verified as accurate: CORS `allow_origins=["*"]` with `allow_credentials=True` (`backend/app/main.py:81`), 30-day token expiry (`backend/app/config.py:7`), default admin credentials (`backend/app/config.py:9`), zero test files in the repo, and no Alembic directory.
## Top 10 — If You Only Fix Ten Things
1. **Fail fast on default secrets** — startup must refuse to run with `SECRET_KEY="change-me-in-production"` or `admin/admin` (SEC-01, BE-15). *S*
2. **Rate-limit + constant-time login** — both login endpoints have unlimited attempts and `!=` comparison (SEC-02, SEC-03). *S*
3. **Restrict CORS** to `https://wealth.cescalante.dev` (SEC-04). *S*
4. **Add a test suite** — zero tests guard the most complex logic in the app (billing-cycle math, deferred carryover, PDF parsing) (ARCH-06, BE-25). *L*
5. **Adopt Alembic** — schema evolution is ad-hoc `ALTER TABLE` strings in `db.py`; memory says "never reset prod DB," which makes real migrations essential (ARCH-02, BE-19). *M*
6. **Verify billing-cycle boundary math** — possible off-by-one in `get_cycle_range` would misalign every monthly budget (BE-06). *S to verify, with tests*
7. **Migrate money fields from `float` to `Decimal`/`Numeric`** (BE-02). *M*
8. **Validate uploads** — size cap, `%PDF` magic-byte check, per-file transactional rollback (SEC-06, BE-10). *S*
9. **Frontend error/feedback layer** — surfaced errors, success toasts, request cancellation; today failures are silent `console.error` (FE-01/02/03, UX-01/02). *M*
10. **n8n sync visibility** — silent parser failures mean missing transactions go unnoticed; add a sync-status/reconciliation view (UX-17, UX-20). *L*
## Reading Order
For a quick pass: this README → [06-weak-spots-inventory.md](06-weak-spots-inventory.md) → [07-improvement-plan.md](07-improvement-plan.md). The five hat reports hold the file:line evidence behind every consolidated item.