admin/Meal-Planner
Public Access
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
d71b67a2976a33a140ac137975638039525497ee
Meal-Planner/Review/synthesis.md
T
admin a0b16f7418 fix: address adversarial review blockers 
All §1 consensus blockers and §2 high-risk gaps resolved:

Schema fixes:
- Remove RecipeIngredient join table, use JSONB for ingredients
- Add family_member table for per-voter approval tracking
- Add all ENUMs for status fields (no loose VARCHAR)
- Add CHECK constraints (household_size, rating 1-5, day_of_week)
- Add name_lower for case-insensitive ingredient matching
- Add grocery_item → ingredient FK
- Fix day_of_week to ISO-8601 (1=Monday, 7=Sunday)
- Remove calorie_target (nutrition is non-goal)

Approval flow redesign:
- Email link → confirmation page (GET), not auto-approve
- Actual vote is POST from confirmation page
- Per-voter tokens (single-use, 72h TTL)
- Record which member voted

Auth model:
- VPN-only for admin endpoints
- Session-based for family web UI

Docker hardening:
- Remove direct port exposure for backend/frontend
- nginx is sole entrypoint
- Add docker-compose.dev.yml for local dev

Skeleton fixes:
- Add missing Pantry.tsx page
- Add missing index.html (Vite entrypoint)
- Add package-lock.json
- Fix SQLAlchemy 2 text() for raw SQL
- Remove create_all from startup (use migrations)
- Configure Alembic properly

Docs updates:
- Update Lucky URL to luckysupermarkets.com
- Add WCAG 2.1 AA accessibility target
- Update family profile with correct mushroom preferences
- Add external dependencies list to SPEC

Verification:
- docker compose config: PASS
- docker compose build backend: PASS
- docker compose build frontend: PASS
- backend import: PASS
- alembic context: PASS
2026-05-04 20:11:05 -07:00

119 lines
10 KiB
Markdown
Raw 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.
# Review Synthesis — Three Reviewers Compared
Sources: `docs-claude.md` (Claude, doc review), `docs-gpt5.5.md` (GPT-5.5, doc review), `repo-gpt5.5.md` (GPT-5.5, repo/code review).
---
## 1. Strong Consensus (all reviewers, blocking)
| # | Issue | Claude | GPT docs | GPT repo |
|---|---|---|---|---|
| C1 | Recipe↔ingredient modeled two ways (JSONB column AND `recipe_ingredient` join) — must pick one | B3 | #2 | #2 (also broken back_populates in code) |
| C2 | No authentication anywhere; admin + approval endpoints publicly mutable | B4 | #4 | #8 |
| C3 | Email approval token model is unsafe (no per-voter scoping, no TTL/single-use, GET mutates state) | B5 | #4 | #9 |
| C4 | `SECRET_KEY` is a ghost/unsafe default with no documented consumer | m6 | #11 | #10 |
| C5 | Schema enums missing — status/reason fields are loose VARCHARs | M4 | (implied #15) | #15 |
| C6 | Docker compose publishes backend (8000) and frontend (3000) directly — bypasses nginx | m3 | — | #12 |
| C7 | Production deployment claims (TLS, rate limiting, prod compose) are not implemented | — | #10 | #13 |
| C8 | Phase status is contradictory — ORIENTATION says Phase 1 complete, plan boxes unticked, code is partial skeleton | m1 | #1 | (confirms repo is skeleton) |
**Implication:** these eight items are the minimum gate before any further code.
---
## 2. Partial Agreement (2 of 3)
| # | Issue | Found by | Missed by |
|---|---|---|---|
| P1 | Migration story muddled — `create_all` at startup vs Alembic-as-source-of-truth; Alembic not configured in repo | GPT docs #5, GPT repo #6 | Claude (only flagged "SQLAlchemy or Alembic" wording — M5) |
| P2 | API endpoint contract conflicts (`POST /api/meals/{id}/approve` vs `GET /api/approve/{token}`, deny variants) | GPT docs #3, GPT repo #7 | Claude did not enumerate route mismatches |
| P3 | Scraping under-specified — feasibility, selectors, anti-bot, normalization | Claude M1, GPT docs #7 | GPT repo (out of scope) |
| P4 | Phase ordering hides risk behind plumbing; need spike for scrape + email before 12-table schema | Claude M2, GPT docs #7 | GPT repo |
| P5 | Shopping-list math (units, conversion, pantry subtraction, "1 bunch cilantro") not implementable from current docs | Claude M4 (partial), GPT docs #9 | GPT repo |
| P6 | Accessibility absent despite family UI + email | Claude m2, GPT docs #16 | GPT repo |
| P7 | Env var docs inconsistent across `.env.example`, ORIENTATION, RUNNING, plan | Claude m6 (SECRET_KEY only), GPT docs #11 | GPT repo |
| P8 | Tech stack drift / "CRA or Vite" wording when project uses Vite | (none) | GPT docs #13, GPT repo (lockfile/index.html) |
| P9 | Health endpoint uses raw `db.execute("SELECT 1")` — needs `text()` under SQLAlchemy 2 | — | GPT docs #14, GPT repo #11 |
---
## 3. Unique Findings (only one reviewer)
### Claude only
- **B1 — Family-profile math doesn't close.** Spec says 2 adults + 2 children, then references "two adults and one child do NOT like mushrooms" + "one adult likes mushrooms" = 3 adults. Single most concrete worked example in the spec, and it doesn't typecheck.
- **B2 — No `family_member` table** despite per-person preferences and asymmetric voting in SPEC §6/§7. Both adults share one approval token; whoever clicks first wins.
- **M3 — APScheduler + multi-worker uvicorn will fire weekly scrape N times.** No leader election, no `--workers 1` pin, no separate scheduler container.
- **M6 — Silence-as-consent.** "If both approve OR no response → confirmed" plus the >80% approval-rate metric means the metric is gamed by spam-foldering.
- **M7 — `calorie_target` column with non-goal status; `spice_level` with no spec mention.** Dead schema or wrong spec.
- **M1 — Lucky URL probably wrong.** `LUCKY_CA_URL=https://www.luckyncal.com`; chain's actual site is `luckysupermarkets.com`. Needs verification.
- **B5.1 — Email GET prefetch.** SafeLinks/Proofpoint/GMail prefetch every link → silent auto-approval before human reads. This is a known failure mode; fix is confirmation page that POSTs.
- **M4 day_of_week 0=Monday** disagrees with both Postgres `EXTRACT(DOW)` (0=Sun) and ISO-8601 (1=Mon).
- **M4 `total_time_minutes`** documented as computed but stored as plain INTEGER — will drift.
- **M4 `ingredient.name UNIQUE`** is case-sensitive → "Carrots" ≠ "carrots".
- **M4 `grocery_item` has no FK to `ingredient`** — the bridge between scraping and planning has no place to live.
- **M8 inline base64 vs CDN** for email images — base64 inflates ~33% and GMail filters >102KB.
### GPT-5.5 docs only
- **#3 (precision)** — exact route-by-route mismatch table between implementation-plan and ARCHITECTURE for approve/deny endpoints.
- **#15** — data-retention windows have no job owner / schedule / FK-cascade design (archive-to-JSON breaks feedback FKs).
- **#17** — observability is aspirational; no log schema, correlation IDs, error taxonomy.
### GPT-5.5 repo only (Claude could not have caught these — doc-only review)
- **#1 — Backend import broken.** `main.py:4` imports `family_profile, ingredient, recipe, …` as submodules; all classes actually live in `models/__init__.py`. **App likely does not start.**
- **#2 — Broken `back_populates`.** `RecipeIngredient.recipe = relationship(..., back_populates="recipe_ingredients")` but `Recipe` has no `recipe_ingredients` attr → mapper config will fail.
- **#3 — Missing `frontend/src/pages/Pantry.tsx`** but imported by `App.tsx:5`.
- **#4 — `npm ci` in Dockerfile but no `package-lock.json`.**
- **#5 — Missing `frontend/index.html`** (Vite entrypoint).
- **#6 — No `alembic.ini` / `alembic/`** despite docs and requirements.
- **#11 — `db.execute("SELECT 1")` will fail under SQLAlchemy 2.**
- **#16 — Trailing-slash inconsistency** between router prefixes and docs.
---
## 4. Where Reviewers Conflict
Almost no direct contradictions — the reviewers are largely orthogonal. Two soft tensions:
1. **Severity of phase-status drift.** Claude treats it as Minor (m1, doc hygiene). GPT-5.5 docs treats it as Blocking (#1). GPT-5.5 repo evidence (skeleton present but broken) supports the GPT-5.5 framing — this should be **Blocking**.
2. **Recipe↔ingredient framing.** Claude frames as "pick one"; GPT-5.5 repo shows code already has *both* with a broken relationship. The decision is no longer abstract — choosing JSONB requires deleting `RecipeIngredient` table; choosing the join requires removing the JSONB column. Either way, code already has to change.
---
## 5. Combined Blocker List (deduplicated, ranked)
Resolve in order. Items 1–6 must land before Phase 2 schema work; 7–9 before any API/feature code.
1. **Reconcile household-vs-members model.** Decide `family_member` table or single shared mailbox. Fixes B1+B2 and unblocks per-voter approval scoping.
2. **Pick recipe-ingredient representation.** Delete the loser from both docs *and* code. Fixes C1 / GPT repo #2.
3. **Define MVP auth.** VPN-only, basic-auth at proxy, or app sessions — pick one and put it in SPEC §8. Fixes C2 + scopes the SECRET_KEY question (C4).
4. **Redesign approval flow.** GET → confirmation page that POSTs; per-voter token; explicit TTL; single-use; record which member voted. Fixes C3 + Claude B5.1.
5. **Reconcile phase status.** One canonical status doc reflecting actual code. Fixes C8.
6. **Stabilize the skeleton (GPT repo Phase 0).** Fix model imports, broken back_populates, missing `index.html` / `Pantry.tsx` / lockfile, `text("SELECT 1")`, configure Alembic, remove `create_all` from startup. Without this, "Phase 1 complete" is false.
7. **Schema cleanup pass:** enums/CHECKs for status fields; `CITEXT` or lowercase for ingredient names; `grocery_item.ingredient_id` FK; CHECK that adult+child = household_size; `GENERATED` column for `total_time_minutes`; pick day_of_week convention; resolve `calorie_target` / `spice_level` vs spec.
8. **Compose hardening:** intra-network only for backend/frontend; nginx is sole entrypoint.
9. **Decide scheduler topology** (separate container, or `--workers 1` pinned and documented) before APScheduler is wired.
---
## 6. Additional Investigation Needed
These were not resolved by any reviewer and require concrete checks:
- [ ] **Verify the Lucky URL.** `curl -I https://www.luckyncal.com` and confirm it serves the expected weekly-ad surface; if wrong, every Phase 4 task is built on sand.
- [ ] **Run the GPT-5.5 verification matrix** (`repo-gpt5.5.md` §"Verification Matrix") to confirm which build/start commands actually fail today. Reviewers asserted the skeleton is broken but did not run it.
- [ ] **Audit Lucky California ToS and recipe-site copyright** before the scraper exists, not after.
- [ ] **Confirm SendGrid (or alternative) is the chosen mail provider** and what the deliverability profile to GMail/iCloud is — silence-as-consent (Claude M6) is dangerous if delivery rate <100%.
- [ ] **Decide whether the project's "self-hosted" framing is load-bearing.** If yes, every external dependency (SendGrid, AI image API, Let's Encrypt, scraped images) must be enumerated in SPEC §8 with an offline-fallback story. If no, drop the framing.
- [ ] **Threat-model the email approval token** end-to-end: leak via forwarded mail, archived inbox, screenshot, mail-scanner prefetch. Claude flagged it; no reviewer wrote a model.
- [ ] **Define the planner's hard-vs-soft constraints and a no-solution fallback** (GPT docs #8). Neither doc nor code has scoring weights or tie-breakers.
- [ ] **Define unit conversion / "1 bunch cilantro" / "to taste" handling** before pantry and shopping-list models freeze.
- [ ] **Decide WCAG target** (2.1 AA per user CLAUDE.md) and which surfaces it covers (web UI yes; email — accessible HTML + plain-text alt).
---
## 7. Net Assessment
The two adversarial doc reviews are largely complementary, not redundant: GPT-5.5 caught contract-level mismatches (routes, env vars, migration tooling) and Claude caught semantic/correctness issues (the family math, the prefetch problem, scheduler concurrency, silence-as-consent). The GPT-5.5 repo review is the only source of evidence about what actually runs — and it indicates the skeleton labeled "Phase 1 complete" likely does not start.
**Combined readiness: ~4/10.** The product intent is clear, but at least three load-bearing design decisions (member model, ingredient representation, auth model) and one infrastructure gap (Alembic + skeleton import bugs) must be resolved before Phase 2.
Reference in New Issue View Git Blame Copy Permalink