`REVIEW-2026-05-04.md`

72 lines
# trademarks-copyright — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **6 patches applied** (2 P0 + 4 P1/P2). **Multiple P0s need Steve** (rotate ADMIN_TOKEN + CRON_TOKEN, CAN-SPAM mailing address, hunter→drops runway). Architecture: NEW shape — `drops-core` (newsletter engine) is the third extraction candidate alongside directory-core + factory-core.

## Patches APPLIED (local — NOT deployed)

| # | File | Issue | Fix |
|---|---|---|---|
| 1 | `src/middleware.ts` | **5 mutating routes were UNAUTH'D**: `/api/drops/send` (blasts email to all subscribers), `/api/drops/compose` (unbounded LLM), `/api/score` (destructive write of `composite_score`), `/api/swot` (4 parallel qwen calls per request), `/api/brands/hunt` (unbounded LLM, no rate limit) | Expanded matcher to gate all 5 routes via existing ADMIN_TOKEN flow |
| 2 | `src/app/api/brands/hunt/route.ts:21` | `maxDiscoveries` from body, no cap — caller could pass 99999 to pin Ollama for hours | `Math.max(1, Math.min(50, requested))` |
| 3 | `src/lib/qwen.ts:6` | OLLAMA_URL defaulted to `http://localhost:11434` — violates standing rule (Mac2 froze 2026-05-02) | Defaults to MS1 `http://192.168.1.133:11434` |
| 4 | `src/app/api/drops/cron/route.ts:20` | CRON_TOKEN compared with plain `!==` (timing-attack vector + inconsistent with admin middleware HMAC compare) | `timingSafeEqual()` from `node:crypto` after length check |
| 5 | `src/lib/qwen.ts:24` | No fetch timeout — stalled Ollama would hang routes for 300s | `signal: AbortSignal.timeout(120_000)` |

`tsc --noEmit` clean.

## P0 NEEDS STEVE (4 actions)

1. **Rotate ADMIN_TOKEN** — currently in `.env.local` line 30 as `dee639798afc6bc1343d3003bd098cf95ee6322318409c15` (live value sitting in plaintext file, one git-init away from being committed). Mint via `/secrets`, route to env, restart.

2. **Rotate CRON_TOKEN** — currently the literal string `change-me-before-public` (line 25). Anyone who can hit :9770 can trigger full Ollama LLM run + mass email send by POSTing `/api/drops/cron?token=change-me-before-public`. Mint random + update via `/secrets` + restart launchd job.

3. **CAN-SPAM compliance** — `DROPS_MAILING_ADDRESS` is commented out in `.env.local` line 34 with the file's own warning about $50,750/email fines. Per US CAN-SPAM Act, every commercial email must include a physical address. **If drops are actually being sent to subscribers right now, this is a legal P0.** Set the env var to your business mailing address before any further send.

4. **Wire hunter → drops runway** (root cause of `com.steve.trademarks-copyright-daily` launchd failure I diagnosed in tick 1):
   - `composeDropForDate` throws `"No fresh candidates"` when `brand_candidates` is empty
   - The launchd job runs `daily-drop.ts` directly but never first triggers the hunter
   - **Fix options:** (a) wrap launchd in a shell script that does `curl -X POST localhost:9770/api/brands/hunt -H "Authorization: Bearer $ADMIN_TOKEN"` before calling `daily-drop.ts`; (b) have `composeDropForDate` fall back gracefully with a minimum drop rather than throwing fatal; (c) add a `lib/drops/preflight.ts` that returns runway days and auto-runs hunter when below threshold.

## P1 / P2 deferred (in REVIEW)

- `src/lib/brandHunter.ts:124` — `whoisInfo` calls `execFile("whois", [domain])` where `domain` comes from Qwen output. `execFile` (not `exec`) prevents shell injection but a model-emitted `-h someserver` would be misread as a flag. Add explicit `^[a-z0-9][a-z0-9.-]*\.[a-z]{2,}$` + `!startsWith("-")` check before calling.
- `src/app/api/drops/drop-items/[id]/route.ts:21-29` — dynamic SQL column construction from `Object.keys`. Today the loop body is a hardcoded allowlist so not exploitable, but pattern is fragile.
- `src/lib/websearch.ts` + `brandHunter.ts` — fetch to `https://${domain}` where domain comes from Qwen. Could hit `localhost.example.com` or `169.254.169.254.com`. Add RFC-1918 + link-local block list (or import factory-core's `safe-fetch.js` once extracted).
- `next.config.js` — no `headers()` export; missing CSP, X-Frame-Options, HSTS.
- `scripts/daily-drop.ts` — no >10% failure-rate alert (per standing rule `feedback_batch_failure_alerts.md`).
- `/api/news` — `q` param has no length cap before forwarding to Google News.
- `src/lib/rateLimit.ts:28` — `($3 || ' minutes')::interval` SQL string concat (safe today since `windowMin` only set internally; document with warning comment).
- `src/app/api/drops/stripe-webhook/route.ts` — verify event.id idempotency table (`stripe_events(id PRIMARY KEY)`) — Stripe retries on 5xx, double-upgrade risk if not deduped.

## Architecture roadmap — NEW shape: `drops-core`

**Architectural Impact: MEDIUM.** Coherent project with clear domain edges. NOT a directory vertical (no provider records). NOT a factory (brandHunter is procedural ETL, not a stage pipeline with critics). It's a **third shape**: opportunity-discovery + curated-newsletter engine.

### Shape classification

| Pattern | Verdict | Reasoning |
|---|---|---|
| `directory-core` consumer? | **NO** | Models opportunities, not providers/practitioners |
| `factory-core` consumer? | **NO** | Procedural crawl-then-classify, not stage→critic→activate |
| Standalone shape | **YES** | "Drops Engine": candidate-source(s) → score → editorial-compose → tier-gated-delivery → tracking |

### `drops-core` extraction candidate

**60% of project surface area is the drops subsystem** (14 of 24 API routes, 5 of 9 page dirs, 389-line `lib/drops.ts`, Stripe + email + tracking). The composition pattern (poolers → freshCandidates → blurb generation → tier-gated render → tracked delivery) is verbatim reusable for any Steve property that wants a daily curated email — bankrupt-leads, animals adoptables, lawyer-directory monetization tips.

Recommend extracting `~/Projects/drops-core/` as a separate npm workspace (NOT folding into directory-core or factory-core).

### Other architecture changes

1. **Split `src/lib/brandHunter.ts` (497 LOC) into 7 files**: hunter/{fetch, whois, uspto, extract, persist, orchestrator, seeds}. 6 distinct responsibilities currently in one untestable file. Tests cover lifecycle + scoring (exemplary) but NOT the hunter, Stripe webhook, or drops compose.
2. **Promote drops to its own route group** — `src/app/(drops)/` + `src/lib/drops/` subdirectory. Currently flat.
3. **SWOT honesty rename** — `swot.ts:50` does `Promise.all` against same Qwen instance with 4 different system prompts. Either lift the 4 prompts into `lib/swot/agents/{strength,weakness,opportunity,threat}.ts` so each can later get its own model/temperature/critique step, OR rename "4-agent SWOT" to "4-prompt SWOT" for honesty. Current naming collides with factory-core's actual stage/critic semantics.
4. **Test gaps to fill**: Stripe webhook signature + idempotency, drops compose + tier gating, email backend selection.

## Files touched

- `/Users/stevestudio2/Projects/trademarks-copyright/src/middleware.ts`
- `/Users/stevestudio2/Projects/trademarks-copyright/src/app/api/brands/hunt/route.ts`
- `/Users/stevestudio2/Projects/trademarks-copyright/src/app/api/drops/cron/route.ts`
- `/Users/stevestudio2/Projects/trademarks-copyright/src/lib/qwen.ts`