mirror of https://github.com/garrytan/gstack.git
fix(plan-eng-review,plan-ceo-review): trim scope per PR review — drop CEO bullets, own section, fewer patterns
PR review feedback on #1071: - Dropped the data-model exception + JSONField bullets from plan-ceo-review entirely. CEO review should stay high-level on "right-sized diff"; data-model pushback belongs to plan-eng-review, which already carries the full checklist. plan-ceo-review/SKILL.md(.tmpl) are now byte-identical to origin/main. - Trimmed both remaining plan-eng-review bullets (data-model exception, JSONField) to single-sentence terseness, matching the style of sibling preferences instead of reading like a paragraph. - Removed cognitive patterns 11-13 (normalize-first, SRP-for-models, structure-beats-blobs) from the numbered "Cognitive Patterns" list — they were more tactical/lower-level than the rest of that list (Larson, McKinley, Conway-style instincts), and duplicated what the Data Model Review now covers operationally. Renumbered 14-18 down to 11-15. - Promoted the Data Model Review Checklist from a "####" subsection tacked onto Architecture Review to its own "### Data model review" section with its own AskUserQuestion/STOP gate, matching Architecture/Code Quality/Test/Performance as a peer rather than an afterthought. Did not renumber the existing 1-4 sections (Architecture/Code Quality/Test/ Performance) since those numbers are referenced elsewhere (carve-guards.ts, the main SKILL.md.tmpl's section-order prose). Updated test/skill-validation.test.ts's static guardrails to match: removed the plan-ceo-review assertions (nothing left to guard there), removed the three now-deleted cognitive-pattern assertions, updated the contiguity check from 1-18 to 1-15, and updated the checklist-subsection test to check for the new "### Data model review" heading. Tightened test/helpers/carve-guards.ts budgets back down now that content shrank: plan-ceo-review reverted fully to its original 90_000/1.08 (content is byte-identical to baseline again); plan-eng-review tightened from 70_000/1.15 to 67_500/1.13 (down from the prior over-generous bump, though not fully back to the original 67_000/no-ratio since the trimmed bullets are still a small net addition). bun test test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — all pass.
This commit is contained in:
David Grant
parent
f3c54078aa
commit
54a50e4b67
|
|
@ -897,8 +897,6 @@ Do NOT make any code changes. Do NOT start implementation. Your only job right n
|
|||
* I err on the side of handling more edge cases, not fewer; thoughtfulness > speed.
|
||||
* Bias toward explicit over clever.
|
||||
* Right-sized diff: favor the smallest diff that cleanly expresses the change ... but don't compress a necessary rewrite into a minimal patch. If the existing foundation is broken, invoke permission #9 and say "scrap it and do this instead."
|
||||
* **Data model exception to "right-sized diff": for schema and model changes, don't let diff size alone decide the shape.** Normalize first; denormalize when you have a measured reason — a profiled hot path, a load test, a documented query-plan cost — and say so in the plan. Splitting a model that's doing two unrelated jobs into two clean models is usually worth the extra table, but it's a judgment call, not an automatic rule: if the "split" model would only ever be read through one FK join, written by exactly one code path, and queried by nothing else, keeping it inline can be the right, deliberate choice too. When tempted to merge new fields into an existing model purely to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split; if the fields genuinely belong to the same concept, don't split just to look normalized. Count concepts, not tables.
|
||||
* **JSONField is not an escape hatch for polymorphism.** When tempted to add a JSONField/HStoreField/payload column to encode variant-specific data (different keys for different kinds) that you can already enumerate and expect to stay stable, promote it to explicit columns instead. JSONField loses FK enforcement, cascade/protect behavior, CheckConstraints on shape, queryability, type system, and self-documentation. Diagnostic: if you find yourself documenting "what keys appear in this JSONField for which variant," you have schema, you just put it in a place where the DB can't help you. This isn't a blanket ban on JSON columns — legitimate uses are common: caching a third-party API's response verbatim (Stripe webhook payloads, OAuth provider profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, or a schema still being discovered (variants that aren't stable yet — revisit once they are). Polymorphism with knowable, stable variants belongs in explicit columns + CheckConstraints; polymorphism you don't control or haven't discovered yet can legitimately stay in JSON.
|
||||
* Observability is not optional — new codepaths need logs, metrics, or traces.
|
||||
* Security is not optional — new codepaths need threat modeling.
|
||||
* Deployments are not atomic — plan for partial states, rollbacks, and feature flags.
|
||||
|
|
|
|||
|
|
@ -86,8 +86,6 @@ Do NOT make any code changes. Do NOT start implementation. Your only job right n
|
|||
* I err on the side of handling more edge cases, not fewer; thoughtfulness > speed.
|
||||
* Bias toward explicit over clever.
|
||||
* Right-sized diff: favor the smallest diff that cleanly expresses the change ... but don't compress a necessary rewrite into a minimal patch. If the existing foundation is broken, invoke permission #9 and say "scrap it and do this instead."
|
||||
* **Data model exception to "right-sized diff": for schema and model changes, don't let diff size alone decide the shape.** Normalize first; denormalize when you have a measured reason — a profiled hot path, a load test, a documented query-plan cost — and say so in the plan. Splitting a model that's doing two unrelated jobs into two clean models is usually worth the extra table, but it's a judgment call, not an automatic rule: if the "split" model would only ever be read through one FK join, written by exactly one code path, and queried by nothing else, keeping it inline can be the right, deliberate choice too. When tempted to merge new fields into an existing model purely to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split; if the fields genuinely belong to the same concept, don't split just to look normalized. Count concepts, not tables.
|
||||
* **JSONField is not an escape hatch for polymorphism.** When tempted to add a JSONField/HStoreField/payload column to encode variant-specific data (different keys for different kinds) that you can already enumerate and expect to stay stable, promote it to explicit columns instead. JSONField loses FK enforcement, cascade/protect behavior, CheckConstraints on shape, queryability, type system, and self-documentation. Diagnostic: if you find yourself documenting "what keys appear in this JSONField for which variant," you have schema, you just put it in a place where the DB can't help you. This isn't a blanket ban on JSON columns — legitimate uses are common: caching a third-party API's response verbatim (Stripe webhook payloads, OAuth provider profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, or a schema still being discovered (variants that aren't stable yet — revisit once they are). Polymorphism with knowable, stable variants belongs in explicit columns + CheckConstraints; polymorphism you don't control or haven't discovered yet can legitimately stay in JSON.
|
||||
* Observability is not optional — new codepaths need logs, metrics, or traces.
|
||||
* Security is not optional — new codepaths need threat modeling.
|
||||
* Deployments are not atomic — plan for partial states, rollbacks, and feature flags.
|
||||
|
|
|
|||
|
|
@ -834,8 +834,8 @@ If the user asks you to compress or the system triggers context compaction: Step
|
|||
* I err on the side of handling more edge cases, not fewer; thoughtfulness > speed.
|
||||
* Bias toward explicit over clever.
|
||||
* Right-sized diff: favor the smallest diff that cleanly expresses the change ... but don't compress a necessary rewrite into a minimal patch. If the existing foundation is broken, say "scrap it and do this instead."
|
||||
* **Data model exception to "right-sized diff": for schema and model changes, don't let diff size alone decide the shape.** Normalize first; denormalize when you have a measured reason — a profiled hot path, a load test, a documented query-plan cost — and say so in the plan. Splitting a model that's doing two unrelated jobs into two clean models is usually worth the extra table, but it's a judgment call, not an automatic rule: if the "split" model would only ever be read through one FK join, written by exactly one code path, and queried by nothing else, keeping it inline can be the right, deliberate choice too. When tempted to merge new fields into an existing model purely to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split; if the fields genuinely belong to the same concept, don't split just to look normalized. Count concepts, not tables.
|
||||
* **JSONField is not an escape hatch for polymorphism.** When tempted to add a JSONField/HStoreField/payload column to encode variant-specific data (different keys for different kinds) that you can already enumerate and expect to stay stable, promote it to explicit columns instead. JSONField loses FK enforcement, cascade/protect behavior, CheckConstraints on shape, queryability, type system, and self-documentation. Diagnostic: if you find yourself documenting "what keys appear in this JSONField for which variant," you have schema, you just put it in a place where the DB can't help you. This isn't a blanket ban on JSON columns — legitimate uses are common: caching a third-party API's response verbatim (Stripe webhook payloads, OAuth provider profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, or a schema still being discovered (variants that aren't stable yet — revisit once they are). Polymorphism with knowable, stable variants belongs in explicit columns + CheckConstraints; polymorphism you don't control or haven't discovered yet can legitimately stay in JSON.
|
||||
* **Data model exception to "right-sized diff":** normalize first, denormalize only for a measured reason (profiled hot path, load test) — count concepts, not tables, but don't split a model over a single trivial field with no independent lifecycle.
|
||||
* **JSONField is not an escape hatch for polymorphism** when variants are known and stable — promote to explicit columns instead (diagnostic: if you're documenting "what keys appear in this JSONField for which variant," it has schema). Genuinely opaque data (third-party payloads, open-ended preferences, still-being-discovered schemas) is a legitimate exception.
|
||||
|
||||
## Cognitive Patterns — How Great Eng Managers Think
|
||||
|
||||
|
|
@ -851,14 +851,11 @@ These are not additional checklist items. They are the instincts that experience
|
|||
8. **Org structure IS architecture** — Conway's Law in practice. Design both intentionally (Skelton/Pais, Team Topologies).
|
||||
9. **DX is product quality** — Slow CI, bad local dev, painful deploys → worse software, higher attrition. Developer experience is a leading indicator.
|
||||
10. **Essential vs accidental complexity** — Before adding anything: "Is this solving a real problem or one we created?" (Brooks, No Silver Bullet).
|
||||
11. **Normalize first, denormalize for measured reasons** — Codd's normal forms (1970) and Knuth's "premature optimization is the root of all evil" (1974) both apply directly to schema design. Denormalizing without measurement is exactly what Knuth warned against — denormalizing WITH a measurement (a profiled query, a load test, an explicit latency budget) is exactly what the same principle allows. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast, but once you've measured that a specific join is the bottleneck, denormalizing that one relationship IS the "make it fast" step working as intended, not a violation.
|
||||
12. **Single Responsibility Principle applies to data models** — A model that's doing audit + balance + notifications is failing SRP. A model that has fields that are only meaningful when other fields have specific values is failing SRP via hidden polymorphism. Split until each model has exactly one job (Martin, Clean Architecture; Codd, normal forms) — unless the "extra job" is a single trivial field with no independent query pattern or lifecycle, in which case call out the tradeoff rather than mandating a split.
|
||||
13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time AND expect that enumeration to stay stable, encode them as explicit columns + CheckConstraints, not as a JSONField/payload bag. The DB can enforce FK integrity, shape constraints, and indexing on columns; it can't on JSON paths. This cuts the other way for polymorphism you don't yet control: third-party blobs, opaque user preferences, and schemas still being discovered are legitimately JSONField. The target is "we know there are 4 kinds and each kind has different fields," not "we don't yet know what shape this will take."
|
||||
14. **Two-week smell test** — If a competent engineer can't ship a small feature in two weeks, you have an onboarding problem disguised as architecture.
|
||||
15. **Glue work awareness** — Recognize invisible coordination work. Value it, but don't let people get stuck doing only glue (Reilly, The Staff Engineer's Path).
|
||||
16. **Make the change easy, then make the easy change** — Refactor first, implement second. Never structural + behavioral changes simultaneously (Beck).
|
||||
17. **Own your code in production** — No wall between dev and ops. "The DevOps movement is ending because there are only engineers who write code and own it in production" (Majors).
|
||||
18. **Error budgets over uptime targets** — SLO of 99.9% = 0.1% downtime *budget to spend on shipping*. Reliability is resource allocation (Google SRE).
|
||||
11. **Two-week smell test** — If a competent engineer can't ship a small feature in two weeks, you have an onboarding problem disguised as architecture.
|
||||
12. **Glue work awareness** — Recognize invisible coordination work. Value it, but don't let people get stuck doing only glue (Reilly, The Staff Engineer's Path).
|
||||
13. **Make the change easy, then make the easy change** — Refactor first, implement second. Never structural + behavioral changes simultaneously (Beck).
|
||||
14. **Own your code in production** — No wall between dev and ops. "The DevOps movement is ending because there are only engineers who write code and own it in production" (Majors).
|
||||
15. **Error budgets over uptime targets** — SLO of 99.9% = 0.1% downtime *budget to spend on shipping*. Reliability is resource allocation (Google SRE).
|
||||
|
||||
When evaluating architecture, think "boring by default." When reviewing tests, think "systems over heroes." When assessing complexity, ask Brooks's question. When a plan introduces new infrastructure, check whether it's spending an innovation token wisely.
|
||||
|
||||
|
|
|
|||
|
|
@ -62,8 +62,8 @@ If the user asks you to compress or the system triggers context compaction: Step
|
|||
* I err on the side of handling more edge cases, not fewer; thoughtfulness > speed.
|
||||
* Bias toward explicit over clever.
|
||||
* Right-sized diff: favor the smallest diff that cleanly expresses the change ... but don't compress a necessary rewrite into a minimal patch. If the existing foundation is broken, say "scrap it and do this instead."
|
||||
* **Data model exception to "right-sized diff": for schema and model changes, don't let diff size alone decide the shape.** Normalize first; denormalize when you have a measured reason — a profiled hot path, a load test, a documented query-plan cost — and say so in the plan. Splitting a model that's doing two unrelated jobs into two clean models is usually worth the extra table, but it's a judgment call, not an automatic rule: if the "split" model would only ever be read through one FK join, written by exactly one code path, and queried by nothing else, keeping it inline can be the right, deliberate choice too. When tempted to merge new fields into an existing model purely to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split; if the fields genuinely belong to the same concept, don't split just to look normalized. Count concepts, not tables.
|
||||
* **JSONField is not an escape hatch for polymorphism.** When tempted to add a JSONField/HStoreField/payload column to encode variant-specific data (different keys for different kinds) that you can already enumerate and expect to stay stable, promote it to explicit columns instead. JSONField loses FK enforcement, cascade/protect behavior, CheckConstraints on shape, queryability, type system, and self-documentation. Diagnostic: if you find yourself documenting "what keys appear in this JSONField for which variant," you have schema, you just put it in a place where the DB can't help you. This isn't a blanket ban on JSON columns — legitimate uses are common: caching a third-party API's response verbatim (Stripe webhook payloads, OAuth provider profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, or a schema still being discovered (variants that aren't stable yet — revisit once they are). Polymorphism with knowable, stable variants belongs in explicit columns + CheckConstraints; polymorphism you don't control or haven't discovered yet can legitimately stay in JSON.
|
||||
* **Data model exception to "right-sized diff":** normalize first, denormalize only for a measured reason (profiled hot path, load test) — count concepts, not tables, but don't split a model over a single trivial field with no independent lifecycle.
|
||||
* **JSONField is not an escape hatch for polymorphism** when variants are known and stable — promote to explicit columns instead (diagnostic: if you're documenting "what keys appear in this JSONField for which variant," it has schema). Genuinely opaque data (third-party payloads, open-ended preferences, still-being-discovered schemas) is a legitimate exception.
|
||||
|
||||
## Cognitive Patterns — How Great Eng Managers Think
|
||||
|
||||
|
|
@ -79,14 +79,11 @@ These are not additional checklist items. They are the instincts that experience
|
|||
8. **Org structure IS architecture** — Conway's Law in practice. Design both intentionally (Skelton/Pais, Team Topologies).
|
||||
9. **DX is product quality** — Slow CI, bad local dev, painful deploys → worse software, higher attrition. Developer experience is a leading indicator.
|
||||
10. **Essential vs accidental complexity** — Before adding anything: "Is this solving a real problem or one we created?" (Brooks, No Silver Bullet).
|
||||
11. **Normalize first, denormalize for measured reasons** — Codd's normal forms (1970) and Knuth's "premature optimization is the root of all evil" (1974) both apply directly to schema design. Denormalizing without measurement is exactly what Knuth warned against — denormalizing WITH a measurement (a profiled query, a load test, an explicit latency budget) is exactly what the same principle allows. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast, but once you've measured that a specific join is the bottleneck, denormalizing that one relationship IS the "make it fast" step working as intended, not a violation.
|
||||
12. **Single Responsibility Principle applies to data models** — A model that's doing audit + balance + notifications is failing SRP. A model that has fields that are only meaningful when other fields have specific values is failing SRP via hidden polymorphism. Split until each model has exactly one job (Martin, Clean Architecture; Codd, normal forms) — unless the "extra job" is a single trivial field with no independent query pattern or lifecycle, in which case call out the tradeoff rather than mandating a split.
|
||||
13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time AND expect that enumeration to stay stable, encode them as explicit columns + CheckConstraints, not as a JSONField/payload bag. The DB can enforce FK integrity, shape constraints, and indexing on columns; it can't on JSON paths. This cuts the other way for polymorphism you don't yet control: third-party blobs, opaque user preferences, and schemas still being discovered are legitimately JSONField. The target is "we know there are 4 kinds and each kind has different fields," not "we don't yet know what shape this will take."
|
||||
14. **Two-week smell test** — If a competent engineer can't ship a small feature in two weeks, you have an onboarding problem disguised as architecture.
|
||||
15. **Glue work awareness** — Recognize invisible coordination work. Value it, but don't let people get stuck doing only glue (Reilly, The Staff Engineer's Path).
|
||||
16. **Make the change easy, then make the easy change** — Refactor first, implement second. Never structural + behavioral changes simultaneously (Beck).
|
||||
17. **Own your code in production** — No wall between dev and ops. "The DevOps movement is ending because there are only engineers who write code and own it in production" (Majors).
|
||||
18. **Error budgets over uptime targets** — SLO of 99.9% = 0.1% downtime *budget to spend on shipping*. Reliability is resource allocation (Google SRE).
|
||||
11. **Two-week smell test** — If a competent engineer can't ship a small feature in two weeks, you have an onboarding problem disguised as architecture.
|
||||
12. **Glue work awareness** — Recognize invisible coordination work. Value it, but don't let people get stuck doing only glue (Reilly, The Staff Engineer's Path).
|
||||
13. **Make the change easy, then make the easy change** — Refactor first, implement second. Never structural + behavioral changes simultaneously (Beck).
|
||||
14. **Own your code in production** — No wall between dev and ops. "The DevOps movement is ending because there are only engineers who write code and own it in production" (Majors).
|
||||
15. **Error budgets over uptime targets** — SLO of 99.9% = 0.1% downtime *budget to spend on shipping*. Reliability is resource allocation (Google SRE).
|
||||
|
||||
When evaluating architecture, think "boring by default." When reviewing tests, think "systems over heroes." When assessing complexity, ask Brooks's question. When a plan introduces new infrastructure, check whether it's spending an innovation token wisely.
|
||||
|
||||
|
|
|
|||
|
|
@ -51,12 +51,17 @@ Evaluate:
|
|||
* Data flow patterns and potential bottlenecks.
|
||||
* Scaling characteristics and single points of failure.
|
||||
* Security architecture (auth, data access, API boundaries).
|
||||
* **Data model honesty:** Does each new model/table have exactly one job? Are there nullable fields whose NULL has semantic meaning ("this row is a different kind of thing")? Are there clusters of columns that always co-vary, suggesting a hidden child model? Does any field on a parent model only have meaning when no child rows exist (parent-field-shadowed-by-child smell)? Is there a JSONField/payload column whose keys-per-variant are knowable and stable at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize — but "in doubt" means genuinely unclear, not "this would add a table." A model that's already cleanly single-purpose, or a denormalization backed by a stated measurement, isn't a smell just because it isn't maximally split.
|
||||
* Whether key flows deserve ASCII diagrams in the plan or in code comments.
|
||||
* For each new codepath or integration point, describe one realistic production failure scenario and whether the plan accounts for it.
|
||||
* **Distribution architecture:** If this introduces a new artifact (binary, package, container), how does it get built, published, and updated? Is the CI/CD pipeline part of the plan or deferred?
|
||||
|
||||
#### Data model review checklist
|
||||
For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Use the preamble's AskUserQuestion Format section. The AskUserQuestion call is a tool_use, not prose — call the tool directly.
|
||||
|
||||
**STOP.** Do NOT proceed to the next review section, edit the plan file with the proposed fix, or call ExitPlanMode until the user responds. An issue with an "obvious fix" is still an issue and still needs explicit user approval before it lands in the plan. Loading the AskUserQuestion schema via ToolSearch and then writing the recommendation as chat prose is the failure mode this gate exists to prevent.
|
||||
|
||||
### Data model review
|
||||
|
||||
**Data model honesty:** Does each new model/table have exactly one job? Are there nullable fields whose NULL has semantic meaning ("this row is a different kind of thing")? Are there clusters of columns that always co-vary, suggesting a hidden child model? Does any field on a parent model only have meaning when no child rows exist (parent-field-shadowed-by-child smell)? Is there a JSONField/payload column whose keys-per-variant are knowable and stable at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize — but "in doubt" means genuinely unclear, not "this would add a table." A model that's already cleanly single-purpose, or a denormalization backed by a stated measurement, isn't a smell just because it isn't maximally split.
|
||||
|
||||
For every new model, model change, or schema migration in this plan, run through this checklist proactively — don't wait for the user to push back on model shape. Each check is a smell; presence of a smell doesn't mean "reject the plan," it means "surface the tradeoff and recommend the normalized alternative."
|
||||
|
||||
|
|
|
|||
|
|
@ -13,12 +13,17 @@ Evaluate:
|
|||
* Data flow patterns and potential bottlenecks.
|
||||
* Scaling characteristics and single points of failure.
|
||||
* Security architecture (auth, data access, API boundaries).
|
||||
* **Data model honesty:** Does each new model/table have exactly one job? Are there nullable fields whose NULL has semantic meaning ("this row is a different kind of thing")? Are there clusters of columns that always co-vary, suggesting a hidden child model? Does any field on a parent model only have meaning when no child rows exist (parent-field-shadowed-by-child smell)? Is there a JSONField/payload column whose keys-per-variant are knowable and stable at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize — but "in doubt" means genuinely unclear, not "this would add a table." A model that's already cleanly single-purpose, or a denormalization backed by a stated measurement, isn't a smell just because it isn't maximally split.
|
||||
* Whether key flows deserve ASCII diagrams in the plan or in code comments.
|
||||
* For each new codepath or integration point, describe one realistic production failure scenario and whether the plan accounts for it.
|
||||
* **Distribution architecture:** If this introduces a new artifact (binary, package, container), how does it get built, published, and updated? Is the CI/CD pipeline part of the plan or deferred?
|
||||
|
||||
#### Data model review checklist
|
||||
For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Use the preamble's AskUserQuestion Format section. The AskUserQuestion call is a tool_use, not prose — call the tool directly.
|
||||
|
||||
**STOP.** Do NOT proceed to the next review section, edit the plan file with the proposed fix, or call ExitPlanMode until the user responds. An issue with an "obvious fix" is still an issue and still needs explicit user approval before it lands in the plan. Loading the AskUserQuestion schema via ToolSearch and then writing the recommendation as chat prose is the failure mode this gate exists to prevent.
|
||||
|
||||
### Data model review
|
||||
|
||||
**Data model honesty:** Does each new model/table have exactly one job? Are there nullable fields whose NULL has semantic meaning ("this row is a different kind of thing")? Are there clusters of columns that always co-vary, suggesting a hidden child model? Does any field on a parent model only have meaning when no child rows exist (parent-field-shadowed-by-child smell)? Is there a JSONField/payload column whose keys-per-variant are knowable and stable at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize — but "in doubt" means genuinely unclear, not "this would add a table." A model that's already cleanly single-purpose, or a denormalization backed by a stated measurement, isn't a smell just because it isn't maximally split.
|
||||
|
||||
For every new model, model change, or schema migration in this plan, run through this checklist proactively — don't wait for the user to push back on model shape. Each check is a smell; presence of a smell doesn't mean "reject the plan," it means "surface the tradeoff and recommend the normalized alternative."
|
||||
|
||||
|
|
|
|||
|
|
@ -144,11 +144,11 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
|||
},
|
||||
behavioral: 'external',
|
||||
externalTest: 'test/skill-e2e-plan-ceo-review-section-loading.test.ts',
|
||||
// Data-model bias fix (garrytan/gstack#1048) adds the "right-sized diff" schema
|
||||
// exception + JSONField-polymorphism warning to the always-loaded preferences
|
||||
// list, then review feedback narrowed both bullets and added legitimate-JSON /
|
||||
// measured-denormalization counterexamples — pushes past the prior 90_000 budget.
|
||||
maxSkeletonBytes: 92_000,
|
||||
// Data-model bias fix (garrytan/gstack#1048): the schema-normalization bullets
|
||||
// were dropped from plan-ceo-review entirely per PR #1071 review feedback (CEO
|
||||
// review stays high-level on "right-sized diff"; data-model pushback belongs to
|
||||
// plan-eng-review) — this skill is back to its original, unchanged budget.
|
||||
maxSkeletonBytes: 90_000,
|
||||
minUnionBytes: 80_000,
|
||||
mustContain: ['SCOPE EXPANSION', 'SELECTIVE EXPANSION', 'HOLD SCOPE', 'SCOPE REDUCTION'],
|
||||
// Default-on Codex outside-voice (codexPreflight block + CODEX_MODE branch
|
||||
|
|
@ -168,23 +168,20 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
|||
},
|
||||
behavioral: 'plan',
|
||||
// v1.2.0 activation lift (shared first-run-guidance preamble) + #2077 ask-first scope gate.
|
||||
// Data-model bias fix (garrytan/gstack#1048) adds the schema-normalization
|
||||
// preference exception + JSONField warning + 3 cognitive patterns (Codd/Knuth/
|
||||
// Beck normalize-first, Martin SRP-for-models, structure-beats-blobs) to the
|
||||
// always-loaded skeleton, then review feedback narrowed the absolute language
|
||||
// and added legitimate-JSON / measured-denormalization counterexamples —
|
||||
// pushes past the prior 67_000 budget.
|
||||
maxSkeletonBytes: 70_000,
|
||||
// Data-model bias fix (garrytan/gstack#1048) adds a terse schema-normalization
|
||||
// preference exception + JSONField warning to the always-loaded skeleton (the
|
||||
// 3 cognitive patterns and the Architecture-review checklist detail were moved
|
||||
// out per PR #1071 review feedback — trimmed to fit within the original budget).
|
||||
maxSkeletonBytes: 67_500,
|
||||
minUnionBytes: 70_000,
|
||||
mustContain: ['Architecture', 'Code Quality', 'Test', 'Performance'],
|
||||
// Cross-cutting preamble growth (v1.57.2.0 AUQ-failure prose fallback + the
|
||||
// decision-memory nudge + the v1.57.4.0 Boil-the-Ocean rename) plus the
|
||||
// default-on Codex outside-voice (codexPreflight block + CODEX_MODE branch
|
||||
// prose, replacing the smaller opt-in question) land this at ~6.6% over the
|
||||
// v1.53.0.0 baseline. Headroom for those intentional additions. The data-model
|
||||
// bias fix (garrytan/gstack#1048) adds another ~6.5% on top for the same
|
||||
// preferences/cognitive-pattern additions noted above.
|
||||
maxSizeRatio: 1.15,
|
||||
// v1.53.0.0 baseline (~6.6%), plus the data-model bias fix's terse remaining
|
||||
// preference bullets (garrytan/gstack#1048) push this to ~12.6% measured.
|
||||
maxSizeRatio: 1.13,
|
||||
},
|
||||
'plan-design-review': {
|
||||
skill: 'plan-design-review',
|
||||
|
|
|
|||
|
|
@ -1985,9 +1985,11 @@ describe('Bundled browser-skills frontmatter contract', () => {
|
|||
// refactor (templates regenerated, sections rewritten), the bias returns silently.
|
||||
// Static grep beats LLM judge here because the real failure mode is "bullet
|
||||
// silently deleted during an unrelated edit," which greps catch in milliseconds.
|
||||
// plan-ceo-review deliberately does NOT carry these bullets (PR #1071 review
|
||||
// feedback: CEO review should stay high-level on "right-sized diff"; data-model
|
||||
// pushback is plan-eng-review's job) — no guardrails for plan-ceo-review here.
|
||||
describe('data-model bias guardrails', () => {
|
||||
const engReview = fs.readFileSync(path.join(ROOT, 'plan-eng-review', 'SKILL.md'), 'utf-8');
|
||||
const ceoReview = fs.readFileSync(path.join(ROOT, 'plan-ceo-review', 'SKILL.md'), 'utf-8');
|
||||
// Architecture review content renders into sections/review-sections.md (the skeleton
|
||||
// points at it), not the always-loaded SKILL.md skeleton — see the Codex-outside-voice
|
||||
// guardrail above for the same pattern.
|
||||
|
|
@ -1996,7 +1998,7 @@ describe('data-model bias guardrails', () => {
|
|||
|
||||
test('plan-eng-review preserves the "data model exception to right-sized diff" bullet', () => {
|
||||
expect(engReview).toContain('Data model exception to "right-sized diff"');
|
||||
expect(engReview).toContain('Count concepts, not tables');
|
||||
expect(engReview).toContain('count concepts, not tables');
|
||||
});
|
||||
|
||||
test('plan-eng-review preserves the JSONField polymorphism warning', () => {
|
||||
|
|
@ -2004,26 +2006,14 @@ describe('data-model bias guardrails', () => {
|
|||
expect(engReview).toContain('what keys appear in this JSONField for which variant');
|
||||
});
|
||||
|
||||
test('plan-eng-review preserves the Normalize-first cognitive pattern', () => {
|
||||
expect(engReview).toContain('Normalize first, denormalize for measured reasons');
|
||||
});
|
||||
|
||||
test('plan-eng-review preserves the SRP-for-data-models cognitive pattern', () => {
|
||||
expect(engReview).toContain('Single Responsibility Principle applies to data models');
|
||||
});
|
||||
|
||||
test('plan-eng-review preserves the Structure-beats-blobs cognitive pattern', () => {
|
||||
expect(engReview).toContain('Structure beats blobs for known polymorphism');
|
||||
});
|
||||
|
||||
test('plan-eng-review Architecture section preserves Data model honesty check', () => {
|
||||
expect(engReviewSections).toContain('Data model honesty');
|
||||
expect(engReviewSections).toContain('parent-field-shadowed-by-child');
|
||||
expect(engReviewSections).toContain('JSONField-hiding-schema');
|
||||
});
|
||||
|
||||
test('plan-eng-review preserves the Data model review checklist subsection', () => {
|
||||
expect(engReviewSections).toContain('Data model review checklist');
|
||||
test('plan-eng-review preserves the Data model review section', () => {
|
||||
expect(engReviewSections).toContain('### Data model review');
|
||||
// A few load-bearing checklist items — if any disappear, the checklist was gutted
|
||||
expect(engReviewSections).toContain('Single Responsibility (SRP for models)');
|
||||
expect(engReviewSections).toContain('Nullable with semantic meaning');
|
||||
|
|
@ -2032,21 +2022,12 @@ describe('data-model bias guardrails', () => {
|
|||
expect(engReviewSections).toContain('Snapshot vs render-live');
|
||||
});
|
||||
|
||||
test('plan-eng-review cognitive patterns list is contiguous 1–18', () => {
|
||||
test('plan-eng-review cognitive patterns list is contiguous 1–15', () => {
|
||||
// If someone inserts/removes a pattern without renumbering, catch it here
|
||||
for (let i = 1; i <= 18; i++) {
|
||||
for (let i = 1; i <= 15; i++) {
|
||||
expect(engReview).toMatch(new RegExp(`^${i}\\. \\*\\*`, 'm'));
|
||||
}
|
||||
// And item 19 should NOT exist (the list ends at 18)
|
||||
expect(engReview).not.toMatch(/^19\. \*\*/m);
|
||||
});
|
||||
|
||||
test('plan-ceo-review preserves the "data model exception to right-sized diff" bullet', () => {
|
||||
expect(ceoReview).toContain('Data model exception to "right-sized diff"');
|
||||
expect(ceoReview).toContain('Count concepts, not tables');
|
||||
});
|
||||
|
||||
test('plan-ceo-review preserves the JSONField polymorphism warning', () => {
|
||||
expect(ceoReview).toContain('JSONField is not an escape hatch for polymorphism');
|
||||
// And item 16 should NOT exist (the list ends at 15)
|
||||
expect(engReview).not.toMatch(/^16\. \*\*/m);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue