Public Access
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
This commit is contained in:
@@ -0,0 +1,205 @@
|
||||
# Review Consensus — Feedback for Originating Agent
|
||||
|
||||
This document consolidates findings from three independent adversarial reviews of the MealPlanner project documentation and current repository state. Use this as the canonical input for the next planning iteration. Every item here was either agreed by multiple reviewers or supported by direct repo evidence.
|
||||
|
||||
Reviewers:
|
||||
- **Claude** — adversarial documentation review (`Review/docs-claude.md`)
|
||||
- **GPT-5.5 (docs)** — adversarial documentation review (`Review/docs-gpt5.5.md`)
|
||||
- **GPT-5.5 (repo)** — repository / code review (`Review/repo-gpt5.5.md`)
|
||||
|
||||
---
|
||||
|
||||
## Headline
|
||||
|
||||
The product intent is clear and the documentation is a coherent first draft, but **three load-bearing design decisions are unresolved** and **the "Phase 1 complete" skeleton likely does not start or build**. Proceeding to Phase 2 (DB & Models) without resolving the items in §1 will produce a schema and migration that contradict the spec in at least three places, and will be reworked within two phases.
|
||||
|
||||
Combined readiness: **~4/10**.
|
||||
|
||||
---
|
||||
|
||||
## 1. Consensus Blockers — must resolve before any further code
|
||||
|
||||
These were flagged by all three reviewers (or by both doc reviewers and confirmed in code).
|
||||
|
||||
### 1.1 Recipe ↔ Ingredient relationship is modeled two contradictory ways
|
||||
- `database-schema.md:66` — `recipe.ingredients JSONB NOT NULL`.
|
||||
- `ARCHITECTURE.md:201, 241` — N:N via `recipe_ingredient` join table.
|
||||
- **Code already has both**, with a broken `RecipeIngredient.recipe = relationship("Recipe", back_populates="recipe_ingredients")` where `Recipe` has no matching attribute (`backend/app/models/__init__.py:247`). Mapper config will fail at runtime.
|
||||
- **Action:** pick one. JSONB precludes the FK indexes needed for the core "find recipes that use this on-sale item" query; the join table makes ingredient substitution / pantry subtraction tractable. Delete the loser from both docs *and* code.
|
||||
|
||||
### 1.2 No authentication exists anywhere in the system
|
||||
- Spec/architecture describe public-internet access via reverse proxy + Let's Encrypt.
|
||||
- Admin endpoints (`POST /api/admin/scrape`, `GET /api/admin/logs`), pantry mutation, family profile edit, and approval/deny endpoints are all unauthenticated.
|
||||
- `SECRET_KEY=change-me-to-a-random-secret-key` in `.env.example` is referenced by no doc.
|
||||
- **Action:** decide MVP auth model — VPN-only (drop the public-internet framing), basic-auth at proxy, or app-level sessions. Put the choice in SPEC §8 before writing any more endpoints. This decision also scopes what `SECRET_KEY` signs.
|
||||
|
||||
### 1.3 Email approval flow is unsafe and probably exploitable
|
||||
- `GET /api/approve/{token}` mutates state. Mail scanners (SafeLinks, Proofpoint, GMail prefetch) will silently auto-approve every meal before the human sees the message. **This is a known, common failure mode.**
|
||||
- One token per `meal_plan_item` shared across both adults — no per-voter accountability; whoever clicks first wins.
|
||||
- `approval_token_expires` is nullable → tokens are permanent absent app logic.
|
||||
- **Action:** redesign — email link leads to a confirmation page that POSTs; per-voter token; explicit TTL; single-use; record which member voted. Depends on §1.4.
|
||||
|
||||
### 1.4 No `family_member` table despite per-person spec
|
||||
- SPEC §6/§7 describes individual preferences and asymmetric voting ("if either denies → swap").
|
||||
- Schema has only `family_profile` (one row per household) and `feedback` with `UNIQUE(meal_plan_item_id)` — one rating per household per meal.
|
||||
- The single worked example in SPEC §6 is **mathematically inconsistent**: 2-adult household but text references three distinct adults' mushroom preferences.
|
||||
- **Action:** decide whether the system models the household as one entity or as members. Either add `family_member(id, family_profile_id, name, role, email)` and key tokens/feedback off it, or downgrade the spec to "one shared mailbox, one shared opinion." This is a 1-line decision that propagates everywhere.
|
||||
|
||||
### 1.5 Phase status is contradictory and the skeleton does not work
|
||||
- `ORIENTATION.md` says Phase 1 complete; `implementation-plan.md` checkboxes are all unticked.
|
||||
- `backend/app/main.py:4` imports `family_profile, ingredient, recipe, …` as submodules from `app.models`, but all classes live in `models/__init__.py`. **Import will fail.**
|
||||
- `frontend/src/App.tsx:5` imports `./pages/Pantry`; the file does not exist. **TS/Vite build will fail.**
|
||||
- `frontend/Dockerfile` runs `npm ci`; no `package-lock.json`. **Frontend container will not build.**
|
||||
- No `frontend/index.html` (Vite entrypoint). **Vite will not start.**
|
||||
- No `backend/alembic.ini` or `backend/alembic/` despite docs requiring `alembic upgrade head`.
|
||||
- `backend/app/main.py:17` calls `Base.metadata.create_all(...)` at startup, undermining migration ownership.
|
||||
- `backend/app/main.py:28` uses `db.execute("SELECT 1")` — must be `text("SELECT 1")` under SQLAlchemy 2.
|
||||
- **Action:** before declaring Phase 1 complete, run a verification matrix that proves the skeleton imports, builds, and starts.
|
||||
|
||||
### 1.6 Schema has avoidable correctness defects
|
||||
- All status/reason fields are loose `VARCHAR(N)` — use Postgres ENUMs or CHECK constraints (otherwise `"approved "` with a trailing space is valid).
|
||||
- `ingredient.name UNIQUE` is case-sensitive — "Carrots" ≠ "carrots". Use `CITEXT` or lowercase-on-write.
|
||||
- `grocery_item` has no FK to `ingredient`. The bridge between scraping and planning has no place to live.
|
||||
- `family_profile.adult_count + child_count` not checked against `household_size`.
|
||||
- `recipe.total_time_minutes` documented as computed but stored as plain INTEGER — will drift. Use `GENERATED ALWAYS AS … STORED`.
|
||||
- `meal_plan_item.day_of_week` 0=Monday disagrees with both Postgres `EXTRACT(DOW)` (0=Sun) and ISO-8601 (1=Mon).
|
||||
- `feedback.UNIQUE(meal_plan_item_id)` blocks per-member ratings (see §1.4).
|
||||
- `calorie_target` exists in schema; nutrition is a stated non-goal. Either drop it or update the spec.
|
||||
|
||||
### 1.7 Docker compose exposes backend (8000) and frontend (3000) directly
|
||||
- `nginx` is described as "the" entrypoint, but ports 8000/3000 are host-published, so any rate limit / auth header / TLS at nginx is bypassable.
|
||||
- **Action:** intra-network only for backend/frontend in prod compose; nginx is sole entrypoint. Consider splitting dev vs prod compose files.
|
||||
|
||||
### 1.8 Production deployment claims exceed implementation
|
||||
- `RUNNING.md` references `docker-compose.prod.yml`, SSL cert mounts, rate limiting — none exist.
|
||||
- `nginx/nginx.conf` only proxies `/api/` and serves SPA fallback over HTTP.
|
||||
- **Action:** label current nginx config as local/dev only; remove or clearly mark unimplemented production commands.
|
||||
|
||||
---
|
||||
|
||||
## 2. High-Risk Gaps — agreed by 2 of 3 reviewers
|
||||
|
||||
### 2.1 Migration tooling is muddled
|
||||
"SQLAlchemy or Alembic" is wrong wording (Alembic *is* SQLAlchemy's tool). Phase 1 commits to Alembic + initial migration before Phase 2 defines the models. Code uses `create_all` at startup. **Action:** define a single migration policy; remove `create_all` from normal startup; configure Alembic in Phase 1 with an empty initial migration and let Phase 2 generate the real one.
|
||||
|
||||
### 2.2 API endpoint contract conflicts across docs
|
||||
`implementation-plan.md` and `ARCHITECTURE.md` disagree on approve/deny route shapes (`POST /api/meals/{id}/approve` vs `GET /api/approve/{token}`, etc.). **Action:** publish one OpenAPI-aligned endpoint table; distinguish authenticated web actions from token-based email actions; specify request/response schemas, status codes, redirects, expiry errors, and idempotency.
|
||||
|
||||
### 2.3 Scraping is under-specified and risky
|
||||
- No feasibility spike, selector strategy, anti-bot policy (Playwright is mentioned, suggesting static fetch is already known to fail).
|
||||
- No Lucky California / recipe-site ToS or copyright analysis.
|
||||
- `LUCKY_CA_URL=https://www.luckyncal.com` may be wrong — the chain's actual site is `luckysupermarkets.com`. **Verify before Phase 4.**
|
||||
- `grocery_item` to `ingredient` mapping (the entire scrape→plan bridge) has no schema support.
|
||||
|
||||
### 2.4 Phase ordering hides risk behind plumbing
|
||||
The risky work — scraping, planner algorithm, email/approval — is queued *after* full backend, full schema, and most of the React UI. Each is most likely to force schema changes; deferring them maximizes rework. **Action:** spike scrape-one-page and email-and-receive-approval-click *before* committing to the 12-table schema.
|
||||
|
||||
### 2.5 Shopping-list math is not implementable from current docs
|
||||
Unit conversion, recipe scaling, pantry subtraction, "1 bunch cilantro", "to taste salt" — all hand-waved. **Action:** define canonical units, conversion tables, rounding rules, and ambiguous-quantity handling before pantry/recipe models freeze.
|
||||
|
||||
### 2.6 Environment variables inconsistent across `.env.example`, ORIENTATION, RUNNING, plan
|
||||
**Action:** one canonical env-var table with name / required / optional / default / scope (backend startup vs frontend build vs optional integration). Add startup validation.
|
||||
|
||||
### 2.7 Accessibility absent despite family-facing UI + email
|
||||
User's global CLAUDE.md mandates WCAG 2.1 AA for UI. **Action:** state the WCAG target in SPEC; cover web UI plus email (accessible HTML + plain-text alt).
|
||||
|
||||
### 2.8 Health check will fail under SQLAlchemy 2
|
||||
`db.execute("SELECT 1")` → use `text("SELECT 1")`.
|
||||
|
||||
---
|
||||
|
||||
## 3. Single-Reviewer Findings worth keeping
|
||||
|
||||
### From Claude
|
||||
- **Silence-as-consent.** "If both approve OR no response → confirmed" combined with the >80% approval-rate metric means the metric is gamed by spam-foldering. State the default explicitly; replace the metric with one that's queryable.
|
||||
- **APScheduler + multi-worker uvicorn** will fire weekly scrape N times if N>1. Pin `--workers 1` and document, or move scheduling to a dedicated container.
|
||||
- **Self-hosting framing is undermined** by SendGrid, AI image API, Let's Encrypt, scraped images, optional Datadog. Either enumerate every external dependency in SPEC §8, or drop the "self-hosted, no external services" framing.
|
||||
- **Image strategy contradicts itself.** SPEC lists "AI image generation as primary source" as a non-goal; ARCHITECTURE/plan build it as fallback. Tighten language. Also: "Embed via CDN URL or inline base64" hides a real decision (base64 inflates ~33%, GMail filters >102KB).
|
||||
- **`spice_level` column with no spec mention** — dead schema or missing spec.
|
||||
- **`meal-planner-plan.md`** described as "may be superseded" in ORIENTATION. Either delete or freeze with a banner.
|
||||
- **Self-referential broken link:** `implementation-plan.md:431` references `docs/implemenation-plan.md` (typo).
|
||||
- **`SECRET_KEY` placeholder** gets copied into real `.env` files unchanged more often than anyone admits — make startup require it in non-dev.
|
||||
|
||||
### From GPT-5.5 (docs)
|
||||
- **Data retention windows** lack job owner / schedule / FK-cascade design. "Archive to JSON, delete rows" for meal plans can break feedback FKs.
|
||||
- **Observability is aspirational.** No log schema, correlation IDs, error taxonomy, alert thresholds. Scraper/email failures are core product risks — should be MVP, not future polish.
|
||||
- **Tech stack drift:** "CRA or Vite" wording in plan; project uses Vite. Remove the CRA option to avoid divergent setup.
|
||||
|
||||
### From GPT-5.5 (repo)
|
||||
- **Trailing-slash inconsistency.** Routers prefixed `/api/profile` with paths `/`, exposing `/api/profile/` rather than `/api/profile`. Decide canonical style and update routes + docs together.
|
||||
- **All API routes are static `{"message": ...}` placeholders** — no schemas, no validation, no DB I/O. Implement one resource end-to-end (probably profile or ingredients) to establish the pattern before fanning out.
|
||||
- **No quality gates exist:** no tests, no lint/format config wired to scripts, no CI yaml. Docs mention pytest/Black/isort/ESLint/Prettier — config and scripts are incomplete.
|
||||
|
||||
---
|
||||
|
||||
## 4. Where Reviewers Disagreed
|
||||
|
||||
Almost no direct contradictions. Two soft tensions:
|
||||
|
||||
1. **Severity of phase-status drift.** Claude rated this Minor (doc hygiene). GPT-5.5 rated it Blocking. Repo evidence (skeleton present but broken imports, missing files, no Alembic) supports **Blocking**. Treat as blocker.
|
||||
2. **Recipe-ingredient framing.** Claude framed as "pick one" abstractly. GPT-5.5 repo found the code already has both with a broken relationship — so the decision is not abstract; code must change either way.
|
||||
|
||||
---
|
||||
|
||||
## 5. Combined Remediation Order
|
||||
|
||||
Resolve in this order. Items 1–6 must land before Phase 2 schema work; 7–9 before any feature code.
|
||||
|
||||
1. **Decide household-vs-members model.** (§1.4) Unblocks per-voter approval, fixes the family-math inconsistency.
|
||||
2. **Pick recipe-ingredient representation.** (§1.1) Delete the loser from docs *and* code.
|
||||
3. **Define MVP auth.** (§1.2) Pick one model; put it in SPEC §8; scope `SECRET_KEY`.
|
||||
4. **Redesign approval flow.** (§1.3) GET → confirmation POST; per-voter token; TTL; single-use.
|
||||
5. **Reconcile phase status.** (§1.5) One canonical status doc reflecting actual code.
|
||||
6. **Stabilize the skeleton.** (§1.5) Fix model imports, broken `back_populates`, add `index.html` / `Pantry.tsx` / lockfile, fix `text("SELECT 1")`, configure Alembic, remove `create_all` from startup. Run the verification matrix below.
|
||||
7. **Schema cleanup pass.** (§1.6) Enums/CHECKs, `CITEXT`, `grocery_item.ingredient_id`, household-size CHECK, `GENERATED` total_time, day_of_week convention, drop or document `calorie_target` / `spice_level`.
|
||||
8. **Compose hardening.** (§1.7) Intra-network only for backend/frontend; nginx sole entrypoint.
|
||||
9. **Decide scheduler topology** before APScheduler is wired (§3 Claude). Separate container or pinned `--workers 1`.
|
||||
|
||||
---
|
||||
|
||||
## 6. Verification Matrix (currently failing — make this pass to claim Phase 1 complete)
|
||||
|
||||
```bash
|
||||
docker compose config
|
||||
docker compose build backend
|
||||
docker compose build frontend
|
||||
docker compose up -d db
|
||||
docker compose run --rm backend python -c "from app.main import app; print(app.title)"
|
||||
docker compose run --rm backend alembic upgrade head
|
||||
docker compose run --rm backend pytest
|
||||
docker compose run --rm frontend npm run build
|
||||
```
|
||||
|
||||
If any of these fail, Phase 1 is not complete regardless of what ORIENTATION.md says.
|
||||
|
||||
---
|
||||
|
||||
## 7. Additional Investigation Still Needed
|
||||
|
||||
No reviewer resolved these — they require concrete checks before proceeding:
|
||||
|
||||
- [ ] **Verify the Lucky URL** (`curl -I https://www.luckyncal.com` and confirm it serves the weekly-ad surface).
|
||||
- [ ] **Run the verification matrix** (§6) and capture which commands actually fail.
|
||||
- [ ] **Audit Lucky California ToS and recipe-site copyright** before the scraper exists.
|
||||
- [ ] **Confirm the chosen mail provider and its deliverability profile** to GMail/iCloud — silence-as-consent is dangerous if delivery rate <100%.
|
||||
- [ ] **Decide whether "self-hosted" framing is load-bearing.** If yes, enumerate every external dependency in SPEC §8 with 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.
|
||||
- [ ] **Define planner hard-vs-soft constraints, scoring weights, tie-breakers, and no-solution fallback.** Add example fixtures (one valid plan, one no-solution case).
|
||||
- [ ] **Define unit conversion / "1 bunch cilantro" / "to taste" rules** before pantry and shopping-list models freeze.
|
||||
- [ ] **Decide WCAG 2.1 AA scope** (web UI + accessible email HTML with plain-text alt).
|
||||
- [ ] **Define backup/restore procedure** for the postgres volume and any cached images.
|
||||
- [ ] **Define CI plan** — minimal smoke tests (backend import, mapper config, frontend build) before adding lint/format configs.
|
||||
|
||||
---
|
||||
|
||||
## 8. Suggested Response Format from Originating Agent
|
||||
|
||||
For each numbered item in §1 and §2, please respond with one of:
|
||||
|
||||
- **Accept + plan:** acknowledge, summarize the chosen resolution, and identify which doc/file will change.
|
||||
- **Reject + rationale:** explain why the finding does not apply, citing primary-source evidence (file:line or external doc).
|
||||
- **Defer + condition:** explain what blocks immediate resolution and what condition triggers it.
|
||||
|
||||
For §3 (single-reviewer findings) and §7 (open investigations), a checkbox-level acknowledgement is sufficient unless the agent disagrees.
|
||||
|
||||
The goal of this loop is to converge on one canonical version of SPEC, ARCHITECTURE, database-schema, and implementation-plan before regenerating the Phase 2 migration.
|
||||
Reference in New Issue
Block a user