`wholivedthere/review.md`

35 lines

## Snapshot
- Three-domain home-history scaffold plus LA County parcel MCP lookup service.
- Stack: Node ESM, MCP SDK, shell deploy scripts, nginx, Postgres schema; datastore: Postgres planned.
- How it's run: MCP via `node src/index.mjs`; product deploy via pm2/nginx scripts.
- Status: stale/incomplete: deploy notes say blocked/awaiting gates.

## Top Risks (P0 — fix this week)
- `.env:2-5`, `scripts/deploy-direct.sh:21-22`, `scripts/deploy-pastdoor.sh:28-29`: committed Google/DPLA/GoDaddy API secrets; DNS/domain takeover and API billing exposure. Revoke/rotate now, remove defaults, keep only `.env.example`.
- `scripts/deploy-direct.sh:60`: deploy path drops `pastdoor` database every run; one mistaken rerun destroys production data. Replace with migration/restore into temp DB plus explicit manual confirmation.
- `mcp-la-records/src/arcgis.mjs:83-93`, `mcp-la-records/src/arcgis.mjs:110`: user input is interpolated into ArcGIS SQL-like `where`; quotes/wildcards can alter query semantics. Validate APN with strict regex, escape `%/_/'`, and reject unsupported address chars.
- `mcp-la-records/src/arcgis.mjs:97`: `limit` is unvalidated and passed as `resultRecordCount`; callers can request huge result sets or invalid counts. Clamp to small integer range, e.g. `1..25`.
- `scripts/schema.sql:58`: generated column uses `now()`, which Postgres rejects because generated expressions must be immutable. Replace with a view or query predicate for stale rows.

## Notable Issues (P1 — fix this month)
- `mcp-la-records/src/arcgis.mjs:39-42`: fetch has no timeout; MCP calls can hang on upstream stalls. Use `AbortController` with a short timeout and surface retryable errors.
- `mcp-la-records/src/arcgis.mjs:99-105`: address lookup swallows every ArcGIS failure and returns `[]`, making outages indistinguishable from “no parcel found.” Track failures and throw if all attempts fail.
- `mcp-la-records/src/index.mjs:69-73`: MCP handler trusts `args` despite only declaring JSON schema; bad/missing values flow into lookup. Add runtime validation before calling adapter.
- `scripts/deploy-pastdoor.sh:55-56`: rsync does not exclude `.env*`; local secrets can be copied to the server. Match `deploy-direct.sh` and exclude env files.
- `scripts/smoke-test.sh:65-68`: treats `404` as acceptable for API endpoints, so missing APIs pass smoke tests. Require expected status per endpoint.
- `PLAN.md` vs `INTEGRATION.md`: architecture says Express/`homedb`, integration says Next.js/stayclaim DB. This will confuse future deploy/debug work. Collapse into one current runbook.

## Polish (P2 — when convenient)
- `mcp-la-records` has no focused tests for APN validation, address normalization, no-match, and upstream failure behavior.
- Root repo has no README with “current source of truth”; PLAN reads aspirational, INTEGRATION reads operational.
- `public/` and `data/` are empty; remove or document intended contents.

## Strengths
- `mcp-la-records/src/arcgis.mjs` keeps source access isolated behind a small adapter, which is the right place to harden validation/caching.
- `LA_GIS_NOTES.md` documents endpoint limitations and performance constraints clearly.
- nginx and deploy scripts make the intended production topology explicit.

## Open questions for the owner
- Are the committed API keys already rotated?
- Is `stayclaim` the real app source of truth, with this repo only infra/MCP?
- Should `deploy-direct.sh` still exist now that it can overwrite production DNS and DB?