plan-eng-review, plan-ceo-review: "minimal diff" preference biases reviewers against schema normalization
- Dominant language
- TypeScript
- Stars
- 133k
- Forks
- 19.9k
- Avg merge
- 18h 46m
- Merged PRs (30d)
- 26
Description
## Problem
The `plan-eng-review` and `plan-ceo-review` skills have an engineering-preferences bullet — *"Minimal diff: achieve the goal with the fewest new abstractions and files touched"* (`plan-eng-review/SKILL.md.tmpl:42`, `plan-ceo-review/SKILL.md.tmpl:58`) — that's sane for bug fixes and most feature work, but biases the reviewing AI against normalization during **schema design**.
Creating a new model means touching more files (model + migration + admin + tests) than adding columns, so "minimal diff" systematically pushes the AI toward:
1. **Table-count minimization** — recommending "add columns to an existing model" when a new model would be cleaner (failing SRP / 3NF).
2. **JSONField-as-polymorphism-escape-hatch** — recommending a \`payload\` JSONField when variants are knowable at design time and belong in explicit columns + \`CheckConstraints\`.
Both are the same root bias: "merge concerns into fewer parts to feel simpler" at the cost of FK enforcement, queryability, DB-level integrity, type safety, and self-documentation.
## Evidence
Caught in a real \`/plan-eng-review\` session where an experienced Django developer had to push back three separate times on the bias — first on table count (proposing to merge new fields into an existing model instead of creating a new one), then on a JSONField for polymorphic notification data instead of explicit FKs + CheckConstraints. Each pushback was unwinding a bias the skill had planted.
The "minimal diff" bullet is correct in other contexts (\`investigate/SKILL.md.tmpl\`, office-hours "minimal viable" alternative) — those shouldn't change. The fix is scoped to the two plan-review skills that review schema decisions.
## Supporting evidence: no counter-language
The cognitive patterns list in \`plan-eng-review\` has 15 patterns covering engineering management (Larson, McKinley, Fowler, Conway, Brooks, SRE), but zero patterns covering data-modeling discourse. There's no Codd, no Knuth, no Beck "make it right, make it fast," no SRP-applied-to-schemas. The bias has no counterweight.
\`ETHOS.md\`'s "Boil the Lake" / Completeness Principle is framed around business logic and tests; it has nothing to say about data modeling.
## Proposed fix
A patch is ready on a branch (\`fix-schema-consolidation-bias\`) that:
1. Adds a **"Data model exception to minimal-diff"** bullet to both skills' preferences lists, explicitly carving out schema design from the minimal-diff default.
2. Adds a **"JSONField is not an escape hatch for polymorphism"** bullet with the diagnostic *"if you find yourself documenting what keys appear in this JSONField for which variant, you have schema — just put it where the DB can't help you."*
3. Adds three new cognitive patterns to \`plan-eng-review\`: *Normalize first, denormalize for measured reasons* (Codd/Knuth/Beck), *SRP applies to data models* (Martin/Codd), *Structure beats blobs for known polymorphism*.
4. Adds a **Data model honesty** check to the Architecture Review evaluation list covering nullable-with-semantic-meaning, column clusters that co-vary, parent-field-shadowed-by-child, and JSONField-hiding-schema smells.
5. Adds a detailed **Data model review checklist** subsection (~60 lines) under Architecture Review covering 11 proactive checks: SRP, nullable-semantic, hidden-models, parent-shadowed, JSONField, FK deletion, snapshot-vs-live, cross-scope FK, derived state, field naming, DB constraints.
6. Adds 10 static guardrails in \`test/skill-validation.test.ts\` that grep the regenerated SKILL.md files for the load-bearing bullets (free, <5ms, catches silent deletion in future refactors).
7. Adds one periodic-tier E2E regression test in \`test/skill-e2e-plan.test.ts\` using a synthetic SubscriptionTier plan that verifies the skill recommends a separate model and pushes back on JSONField.
Unchanged on purpose: \`investigate/\`, \`openclaw-investigate/\` (minimal-diff is correct for bug fixes), \`office-hours/\` (minimal-viable as one of three alternatives is fine).
## References
- Codd 1970 (normal forms)
- Knuth 1974 ("premature optimization is the root of all evil")
- Beck ("make it work, make it right, make it fast")
- Martin (SOLID/SRP)
Contributor guide
Assessment
This issue has not been assessed yet.