cfn-plan-review

# CFN Plan Review

**Purpose:** Catch what plans miss. Plans fail when they scope from the task description instead of from the system. This skill forces investigation of what the plan takes for granted.

## When to Use

Run after any plan is written and before implementation begins. Especially critical for:
- Database migrations or schema changes
- Cross-project data movement
- API contract changes
- Service decomposition or consolidation
- Any plan where "just move X" is the framing

## Protocol

### Phase 0: DRY & Modularity Check

Before reviewing completeness, apply the DRY and modularity rules from `~/.claude/rules/code-quality.md` (DRY & Modularity section) to the plan:

- Does the plan introduce logic that already exists elsewhere?
- Does it touch 8+ files? If so, pause and verify minimum viable scope.
- Are there shared types, schemas, or constants that need a single source of truth?
- Does any multi-file feature have a shared orchestrator, or are there multiple entry points?

Surface any violations as numbered findings in Phase 6. Do not duplicate the rules here. Consult `code-quality.md` directly.

### Phase 1: Assumption Extraction

Before extracting assumptions, query the decision log for prior plans involving the same entities: `~/.claude/skills/decision-log/query.sh '<entity-names>' 5 <project>`. Prior failed assumptions from past plans should be checked first.

Read the plan and extract every implicit assumption into an explicit, testable statement.

Common hidden assumptions:
- "This entity is self-contained" (it almost never is)
- "Nothing else reads/writes this table" (check for views, functions, cron jobs, other services)
- "The schema matches what I expect" (dump it and verify)
- "This API is only called by one consumer" (grep for the endpoint across all projects)
- "The data fits in memory / can be migrated in one pass" (check row counts)
- "Existing data is clean and consistent" (check for nulls, orphans, constraint violations)
- "My custom header reaches the backend" (check proxy/gateway header whitelists; Next.js catch-all routes, nginx, and API gateways silently strip unknown headers)

Output format:
```
## Assumptions
1. [UNTESTED] Listings table has no FK dependencies on other tables
2. [UNTESTED] No other service writes to the golfer_profiles table
3. [VERIFIED] The target database already has the uuid-ossp extension
```

Each assumption is UNTESTED until explicitly verified by querying the system.

### Phase 2: Dependency Trace

For every entity the plan touches (table, API, module, config), trace dependencies in both directions:

**Inbound (what does this entity need to exist?):**
- FK references to other tables
- Required lookup/reference data
- Config values, environment variables
- Shared types or schemas it imports
- Services it calls

**Outbound (what depends on this entity?):**
- Other tables with FKs pointing here
- Views, functions, triggers that reference it
- Services that query or subscribe to it
- Cron jobs, pipelines, background workers
- Frontend code that renders its data
- API consumers (internal and external)

**For API/HTTP changes, also check the request path:**
- Does the request pass through a reverse proxy, API gateway, or frontend proxy before reaching the backend? (e.g., Next.js catch-all route, nginx, Cloudflare Workers)
- Does the proxy have a header whitelist? Custom auth headers (X-Research-Key, X-Custom-Auth, etc.) are silently stripped by proxies that only forward known headers. This causes 401s that are impossible to reproduce when testing the backend directly.
- Are there middleware layers (CORS, CSRF, auth) mounted at the router level in server.ts that don't appear in the route file itself? Check `app.use()` calls, not just per-route middleware.

For database operations, the investigation MUST include:
```sql
-- Trace FKs pointing TO this table
SELECT conrelid::regclass, conname, pg_get_constraintdef(oid)
FROM pg_constraint
WHERE confrelid = 'target_table'::regclass;

-- Trace FKs pointing FROM this table
SELECT confrelid::regclass, conname, pg_get_constraintdef(oid)
FROM pg_constraint
WHERE conrelid = 'target_table'::regclass AND contype = 'f';

-- Check for views referencing this table
SELECT viewname FROM pg_views
WHERE definition LIKE '%target_table%';

-- Check for functions referencing this table
SELECT proname FROM pg_proc
WHERE prosrc LIKE '%target_table%';
```

Output format:
```
## Dependency Graph: listings

### Needs (inbound)
- golfer_profiles (FK: listings.golfer_id -> golfer_profiles.id)
- courses (FK: listings.course_id -> courses.id)
- listing_types (FK: listings.type_id -> listing_types.id)
- pricing_tiers (referenced in listings.compute_price())

### Needed By (outbound)
- listing_images (FK: listing_images.listing_id -> listings.id)
- bookings (FK: bookings.listing_id -> listings.id)
- search_index (materialized view, refreshes from listings)
- daily-seo cron job (reads listings for sitemap generation)
```

### Phase 3: Blast Radius

Answer: "If we ship exactly this plan and nothing else, what breaks?"

For each dependency found in Phase 2, evaluate:
1. Is it covered by the plan? (explicitly handled)
2. Is it out of scope but safe? (no impact from this change)
3. Is it a gap? (will break if not addressed)

Output format:
```
## Blast Radius

### Covered by plan
- listings table migration [Phase 2 of plan]
- listing_images migration [Phase 3 of plan]

### Safe (no action needed)
- search_index view (will be recreated in target DB)

### GAPS (will break)
- golfer_profiles table NOT in migration plan but listings FK requires it
- pricing_tiers NOT in plan but listings.compute_price() references it
- bookings table NOT in plan but has FK to listings
```

### Phase 4: Edge Cases

Surface scenarios the plan does not address:
- What happens to in-flight data during migration? (rows written between snapshot and cutover)
- What happens to orphaned records? (FKs that reference deleted rows)
- What is the rollback path if migration fails halfway?
- Are there data volume concerns? (100 rows vs 10M rows changes the approach)
- Are there ordering constraints? (table A must exist before table B due to FKs)
- Does the plan assume downtime? If not, how is consistency maintained?

### Phase 5: Alpha Readiness Check

Whatever the plan implements MUST be at least alpha-ready when merged. Alpha-ready = could ship to real users behind a feature flag without on-call paging or data loss. Score the plan against the same 8 dimensions used by `cfn-alpha-launch`. Each dimension is PASS, GAP, or N/A. Any GAP becomes a numbered finding in Phase 6.

**Hard requirements (any miss = BLOCKER):**

| Area | Requirement | Check |
|------|-------------|-------|
| **test** | TDD plan present | Each implementation step names the failing test written first. Bug fixes name the reproducing test. |
| **test** | Regression coverage | Edge cases from Phase 4 each map to a test (unit, integration, or e2e). |
| **security** | RLS on new tables | Every new Supabase table has a Row Level Security policy in the same migration. |
| **security** | Auth boundaries | New endpoints state the auth check (Clerk session, API key, public). No "TBD auth". |
| **security** | No secrets in code | Plan does not hardcode tokens, keys, or DB URLs. Secrets routed via Fly secrets or env. |
| **security** | Headers + RLS audit | New HTTP routes inherit HSTS/CSP/X-Frame-Options via shared middleware; not bypassed. |
| **backend** | Error handling at boundaries | External API calls, DB queries, and user input have explicit error paths. |
| **backend** | No unscoped DELETE/TRUNCATE | Any DELETE in test setup/teardown or migration has WHERE clause targeting test rows only. |
| **frontend** | UI verification step | If frontend touched, plan includes Playwright or manual browser check (golden path + 1 edge case). |
| **architect** | Rollback path | Plan states how to undo if alpha users hit a blocker (revert migration, feature flag off, redeploy prior tag). |
| **supabase** | Migration reversibility | New migrations have `down` direction OR explicitly documented why they cannot be rolled back. |
| **supabase** | Schema sync step | Plan ends with `~/.claude/skills/supabase-schema-sync/execute.sh` after any migration. |
| **contract** | Inter-service typing | Cross-service calls (API, trigger payload, queue message) define a shared Zod schema or TS interface. |
| **contract** | Enum completeness | New enum values traced through ALL consumers (DB, switch/match, serializers, UI). |
| **consistency** | Canonical constants | No hardcoded path/schema/limit strings duplicated across files; routed through shared config. |
| **consistency** | Doc updates | Plan includes updates to `readme/feature-status.md` and `readme/state-machines.md` if entity is stateful. |

**Deployment readiness (Fly.io specific):**

- Static-export apps (Next.js export, Expo web): all `NEXT_PUBLIC_*` / `EXPO_PUBLIC_*` env vars added as Docker build args in `fly.toml` AND `ARG`/`ENV` in Dockerfile.
- SSR/ISR apps: env vars added as Fly secrets via `fly secrets set`.
- Plan includes post-deploy verification: `curl` of key page (not just `/health`) confirms real content renders.
- If touching blog/SEO: plan includes GSC validation timing (no "Validate Fix" same day as deploy).

**Observability:**

- Decision points (auth allow/deny, gate pass/fail, retry vs abort) have a log line with enough context to debug a paged incident.
- Errors include the entity ID, the request ID, and the user/tenant ID where available.
- Plan names the dashboard or log query someone on-call would run to see if this feature is healthy.

**Anti-patterns that auto-fail this phase:**

- "We'll add tests after"
- "Auth will be added later"
- "RLS in a follow-up migration"
- "Will document once stable"
- "Skip rollback path, we'll redeploy"
- Any Anthropic API call in project code (BANNED per `~/.claude/CLAUDE.md`)
- `claude -p` invocation without `--budget` cap or `unset ANTHROPIC_API_KEY`

Output format:
```
## Alpha Readiness Check

| Area | Status | Notes |
|------|--------|-------|
| test | GAP | No failing test specified for new pricing logic |
| security | PASS | RLS policy in same migration, auth via Clerk session |
| backend | GAP | External Stripe call has no error handler |
| frontend | N/A | Backend-only change |
| architect | PASS | Rollback = feature flag off + revert migration 0123 |
| supabase | GAP | Migration missing schema-sync step |
| contract | PASS | Shared Zod schema in packages/contracts |
| consistency | GAP | Doc updates missing |

**Alpha-ready: NO** (4 gaps blocking)
```

### Phase 6: Findings Summary

Present all gaps from Phases 1-5 as numbered questions, one per issue. Each question includes:
- What was found
- Why it matters
- Source phase (assumption / dependency / blast radius / edge case / alpha readiness)
- A recommended action

Format:
```
## Plan Review Findings

1. **golfer_profiles table missing from migration** [Phase 3: blast radius]
   The listings table has an FK to golfer_profiles.id. Migrating listings without golfer_profiles will fail on insert due to FK constraint violation.
   Recommendation: Add golfer_profiles to migration scope, execute before listings.

2. **Assumption untested: no other service writes to listings** [Phase 1: assumption]
   The plan assumes daily-seo is the only writer. If golfer-collective still has write access during migration, data will diverge.
   Recommendation: Verify by checking database connection logs or revoking golfer-collective write access before cutover.

3. **No RLS policy on new pricing_tiers table** [Phase 5: alpha readiness: security]
   Plan creates pricing_tiers without RLS. Per CFN security rules, every new Supabase table requires RLS before deployment.
   Recommendation: Add RLS policy in the same migration. Default-deny + per-tenant allow.

4. **No failing test for compute_price() change** [Phase 5: alpha readiness: test]
   Plan modifies pricing logic but does not name the failing test written first. Per TDD protocol, no implementation without a failing test.
   Recommendation: Add Phase 1.5 to plan: write failing test for new pricing rule before touching production code.
```

## Integration

- Preceded by `cfn-goap-plan` (goal state modeling, A* action sequence) for non-trivial tasks
- Run after `/write-plan` or any Plan Mode session
- Feeds into cfn-investigate if gaps reveal deeper issues
- Log significant findings to cfn-knowledge-base for future reference
- Works with any project type (not database-specific, but database examples are most common)

## What This Skill Does NOT Do

- Does not rewrite the plan. It reviews and surfaces gaps.
- Does not make scope decisions. It presents findings; the user decides.
- Does not run migrations or make changes. It is read-only investigation.