From abafb38137077ec8c88b3a8e9890dd48cb0a7b10 Mon Sep 17 00:00:00 2001 From: David Grant Date: Fri, 17 Apr 2026 15:54:11 -0700 Subject: [PATCH 01/17] fix: counter model-design bias toward fewer tables and JSONField shortcuts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "minimal diff" engineering preference in plan-eng-review and plan-ceo-review is correct for bug fixes but systematically biased AI reviewers against schema normalization: creating a new model touches more files than adding columns, so the preference pushed recommendations toward merging concerns into existing tables and using JSONField as a polymorphism escape hatch. Both failures share one root — "fewer parts feels simpler" at the cost of FK integrity, DB-level constraints, queryability, and type safety. Template changes (both skills' preferences lists): * Data model exception to minimal-diff — schema design normalizes first, denormalizes for measured reasons, counts concepts not tables. * JSONField is not an escape hatch for polymorphism — promote to explicit columns + CheckConstraints when variants are knowable at design time. plan-eng-review cognitive patterns (renumbered 1–18): * Normalize first, denormalize for measured reasons (Codd/Knuth/Beck). * Single Responsibility Principle applies to data models (Martin/Codd). * Structure beats blobs for known polymorphism. plan-eng-review Architecture section: * Data model honesty check covering nullable-semantic, column clusters, parent-field-shadowed-by-child, JSONField-hiding-schema smells. * Full Data model review checklist subsection — 11 proactive checks (SRP, nullable-semantic, hidden-models, parent-shadowed, JSONField, FK deletion, snapshot-vs-live, cross-scope, derived state, field naming, DB constraints). Regression tests: * 10 static grep guardrails in test/skill-validation.test.ts verify every load-bearing bullet survives future refactors (free, <5ms, catches silent deletion). Cognitive-patterns list contiguity (1–18) is asserted. * New E2E case in test/skill-e2e-plan.test.ts (periodic tier) uses a synthetic SubscriptionTier plan to verify the skill recommends a separate model, pushes back on JSONField, and cites normalization/SRP. Unchanged on purpose: investigate/, openclaw-investigate/ (minimal-diff correct in bug-fix context), office-hours/ (minimal-viable as one of three alternatives is fine), openclaw-ceo-review/ (doesn't contain the bullet). Refs: Codd 1970 (normal forms), Knuth 1974 ("premature optimization"), Beck ("make it work, make it right, make it fast"), Martin (SOLID/SRP). Closes garrytan/gstack#1048 Co-Authored-By: Claude Opus 4.7 (1M context) --- plan-ceo-review/SKILL.md | 2 + plan-ceo-review/SKILL.md.tmpl | 2 + plan-eng-review/SKILL.md | 76 +++++++++++++++++-- plan-eng-review/SKILL.md.tmpl | 76 +++++++++++++++++-- test/helpers/touchfiles.ts | 2 + test/skill-e2e-plan.test.ts | 135 ++++++++++++++++++++++++++++++++++ test/skill-validation.test.ts | 67 +++++++++++++++++ 7 files changed, 350 insertions(+), 10 deletions(-) diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index c2fc9bbb6..593837f10 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -645,6 +645,8 @@ 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. * Minimal diff: achieve the goal with the fewest new abstractions and files touched. +* **Data model exception to "minimal diff": for schema and model changes, minimal-diff is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. 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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. * 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. diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index d128b1802..45d21b897 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -61,6 +61,8 @@ 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. * Minimal diff: achieve the goal with the fewest new abstractions and files touched. +* **Data model exception to "minimal diff": for schema and model changes, minimal-diff is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. 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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. * 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. diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index 1b2482e14..f321f5f5c 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -590,6 +590,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. * Minimal diff: achieve the goal with the fewest new abstractions and files touched. +* **Data model exception to "minimal diff": for schema and model changes, minimal-diff is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. Count concepts, not tables. The default for schema design is more clean parts, not fewer muddled ones. +* **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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. ## Cognitive Patterns — How Great Eng Managers Think @@ -605,11 +607,14 @@ 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. **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). +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. The default for data model design is more clean parts, not fewer muddled ones. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast. +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). +13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time, 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. JSONField is for genuinely opaque data (third-party blobs, user preferences nobody queries on), not for "we know there are 4 kinds and each kind has different fields." +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). 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. @@ -766,10 +771,71 @@ 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 at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize. Three clean models > one muddled model in all respects. * 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 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." + +**Single Responsibility (SRP for models)** +- Does this model have exactly one job? Audit ≠ balance ≠ notification ≠ display. +- If a model is doing N jobs, propose splitting it into N models. + +**Nullable with semantic meaning** +- For every nullable column, ask: does NULL flip what kind of thing this row is? +- If yes, the model is hiding polymorphism behind sentinels. Decompose or add a CheckConstraint tying the nullability to an explicit type column. +- Always-meaningful nullables (timestamps, optional notes) are fine. + +**Models hiding as columns** +- For every cluster of 2+ columns, ask: do these always co-vary? Would I ever want column A without column B? +- If they always co-vary, they belong in their own model. +- For every column, ask: is this only meaningful when another column has a specific value? +- If yes, you have polymorphism. Promote to a child model or use a CheckConstraint. + +**Parent-field-shadowed-by-child** +- For every field on a parent model, ask: would a child row of this parent have the same field with case-specific values? +- If yes, the parent's field belongs on the child. Remove from parent, require ≥1 child row, read via FK navigation. + +**JSONField as polymorphism escape hatch** +- For every JSONField, ask: am I documenting "what keys appear for which variant"? +- If yes, the JSONField has schema. Promote to explicit columns + CheckConstraints. +- Legitimate JSONField uses: third-party API caches, opaque user preferences, schemaless attribute bags nobody queries against. + +**FK deletion strategies (CASCADE / PROTECT / SET_NULL)** +- For every FK, ask: if the parent is deleted, what should happen to this row? +- CASCADE only when the child only exists BECAUSE of the parent. +- PROTECT when deleting the parent would silently destroy user-facing state. +- SET_NULL when the child should outlive the parent (audit/log rows). +- Never accept the ORM default without thinking. + +**Snapshot vs render-live** +- For every value stored in a model, ask: is this a NUMERIC INPUT to a calculation, or DISPLAY COPY? +- Numeric inputs to calculations should be SNAPSHOTTED at the moment of the calculation (so math is reproducible later, immune to source drift). +- Display copy should be rendered at view time from current FK chains (so renames retroactively update history). +- Mixing the two is a smell. + +**Cross-scope FK consistency** +- For every model with FKs spanning a "scope" (household, tenant, organization), the DB does not enforce that all FKs resolve to the same scope. +- Add a validation check that all FKs resolve to the same scope. +- Single-tenant doesn't need it but adding it now is cheap and future-proofs. + +**Derived state beats stored state when read is cheap** +- For every column that could be derived from another column or query, ask: is the derivation cost acceptable for the read pattern? +- If yes, derive (single source of truth, can't disagree). +- Only store derived state when read cost is unacceptable. + +**Field naming** +- For every field, ask: does the name describe WHY it exists (semantics) or HOW it works (implementation)? +- Bad names: order, position, sequence, type, status, value, data, payload, info, meta. +- Good names: domain words for the actual concept (step, kind, learning_bonus, role). + +**Validation: DB constraints over app validation** +- For every cross-field invariant, prefer a DB CheckConstraint over app-layer validation. +- Bulk-insert paths often skip app-layer validation entirely. CheckConstraints catch bugs at write time regardless of how the row was inserted. + **STOP.** 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. Only proceed to the next section after ALL issues in this section are resolved. ## Confidence Calibration diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index dab83e72b..fe018d1df 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -46,6 +46,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. * Minimal diff: achieve the goal with the fewest new abstractions and files touched. +* **Data model exception to "minimal diff": for schema and model changes, minimal-diff is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. Count concepts, not tables. The default for schema design is more clean parts, not fewer muddled ones. +* **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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. ## Cognitive Patterns — How Great Eng Managers Think @@ -61,11 +63,14 @@ 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. **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). +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. The default for data model design is more clean parts, not fewer muddled ones. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast. +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). +13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time, 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. JSONField is for genuinely opaque data (third-party blobs, user preferences nobody queries on), not for "we know there are 4 kinds and each kind has different fields." +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). 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. @@ -131,10 +136,71 @@ 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 at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize. Three clean models > one muddled model in all respects. * 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 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." + +**Single Responsibility (SRP for models)** +- Does this model have exactly one job? Audit ≠ balance ≠ notification ≠ display. +- If a model is doing N jobs, propose splitting it into N models. + +**Nullable with semantic meaning** +- For every nullable column, ask: does NULL flip what kind of thing this row is? +- If yes, the model is hiding polymorphism behind sentinels. Decompose or add a CheckConstraint tying the nullability to an explicit type column. +- Always-meaningful nullables (timestamps, optional notes) are fine. + +**Models hiding as columns** +- For every cluster of 2+ columns, ask: do these always co-vary? Would I ever want column A without column B? +- If they always co-vary, they belong in their own model. +- For every column, ask: is this only meaningful when another column has a specific value? +- If yes, you have polymorphism. Promote to a child model or use a CheckConstraint. + +**Parent-field-shadowed-by-child** +- For every field on a parent model, ask: would a child row of this parent have the same field with case-specific values? +- If yes, the parent's field belongs on the child. Remove from parent, require ≥1 child row, read via FK navigation. + +**JSONField as polymorphism escape hatch** +- For every JSONField, ask: am I documenting "what keys appear for which variant"? +- If yes, the JSONField has schema. Promote to explicit columns + CheckConstraints. +- Legitimate JSONField uses: third-party API caches, opaque user preferences, schemaless attribute bags nobody queries against. + +**FK deletion strategies (CASCADE / PROTECT / SET_NULL)** +- For every FK, ask: if the parent is deleted, what should happen to this row? +- CASCADE only when the child only exists BECAUSE of the parent. +- PROTECT when deleting the parent would silently destroy user-facing state. +- SET_NULL when the child should outlive the parent (audit/log rows). +- Never accept the ORM default without thinking. + +**Snapshot vs render-live** +- For every value stored in a model, ask: is this a NUMERIC INPUT to a calculation, or DISPLAY COPY? +- Numeric inputs to calculations should be SNAPSHOTTED at the moment of the calculation (so math is reproducible later, immune to source drift). +- Display copy should be rendered at view time from current FK chains (so renames retroactively update history). +- Mixing the two is a smell. + +**Cross-scope FK consistency** +- For every model with FKs spanning a "scope" (household, tenant, organization), the DB does not enforce that all FKs resolve to the same scope. +- Add a validation check that all FKs resolve to the same scope. +- Single-tenant doesn't need it but adding it now is cheap and future-proofs. + +**Derived state beats stored state when read is cheap** +- For every column that could be derived from another column or query, ask: is the derivation cost acceptable for the read pattern? +- If yes, derive (single source of truth, can't disagree). +- Only store derived state when read cost is unacceptable. + +**Field naming** +- For every field, ask: does the name describe WHY it exists (semantics) or HOW it works (implementation)? +- Bad names: order, position, sequence, type, status, value, data, payload, info, meta. +- Good names: domain words for the actual concept (step, kind, learning_bonus, role). + +**Validation: DB constraints over app validation** +- For every cross-field invariant, prefer a DB CheckConstraint over app-layer validation. +- Bulk-insert paths often skip app-layer validation entirely. CheckConstraints catch bugs at write time regardless of how the row was inserted. + **STOP.** 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. Only proceed to the next section after ALL issues in this section are resolved. {{CONFIDENCE_CALIBRATION}} diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index 34ead7d0c..ba68cad61 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -77,6 +77,7 @@ export const E2E_TOUCHFILES: Record = { 'plan-ceo-review-benefits': ['plan-ceo-review/**', 'scripts/gen-skill-docs.ts'], 'plan-eng-review': ['plan-eng-review/**'], 'plan-eng-review-artifact': ['plan-eng-review/**'], + 'plan-eng-review-data-model-bias': ['plan-eng-review/**'], 'plan-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'], // Codex offering verification @@ -236,6 +237,7 @@ export const E2E_TIERS: Record = { 'plan-ceo-review-benefits': 'gate', 'plan-eng-review': 'periodic', 'plan-eng-review-artifact': 'periodic', + 'plan-eng-review-data-model-bias': 'periodic', 'plan-eng-coverage-audit': 'gate', 'plan-review-report': 'gate', diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 8953200b1..7bb9694dd 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -277,6 +277,141 @@ Focus on architecture, code quality, tests, and performance sections.`, }, 420_000); }); +// --- Plan Eng Review Data-Model Bias Regression E2E --- +// +// Regression test for the data-model bias fix: verifies that /plan-eng-review +// recommends a separate model instead of inlining columns + JSONField when +// a plan tries to merge polymorphic variant data into an existing model "to +// keep the diff minimal." Before the fix, the skill's "minimal diff" preference +// pushed the AI toward accepting the inline approach; after the fix, the +// data-model exception bullet + Data model review checklist should make the +// AI push back and recommend normalization citing SRP/3NF. + +describeIfSelected('Plan Eng Review Data-Model Bias E2E', ['plan-eng-review-data-model-bias'], () => { + let planDir: string; + + beforeAll(() => { + planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-dmb-')); + const run = (cmd: string, args: string[]) => + spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); + + run('git', ['init', '-b', 'main']); + run('git', ['config', 'user.email', 'test@test.com']); + run('git', ['config', 'user.name', 'Test']); + + // Synthetic plan designed to trip the old bias: four clearly-polymorphic + // tier variants + feature-flag bag that the user proposes to inline onto + // User rather than create a new model. After the fix, the skill should + // recommend a separate SubscriptionTier model and push back on the + // JSONField for tier_features. + fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add Subscription Tiers to User Model + +## Context +I want to add a 'subscription tier' feature to my Django app. Each user can +be on a free, basic, premium, or enterprise tier. Each tier has: +- a monthly price +- a feature set (list of feature flags) +- a max-users limit +- an SLA tier (none / standard / premium) + +## Current state +The User model has ~15 fields (email, name, created_at, etc). + +## Proposed changes +I'm thinking of just adding these columns directly to the User model so I +don't have to deal with another table: + +- tier_name: CharField with choices=['free', 'basic', 'premium', 'enterprise'] +- tier_price: DecimalField +- tier_features: JSONField (list of feature flag strings, varies per tier) +- tier_max_users: IntegerField +- tier_sla: CharField with choices=['none', 'standard', 'premium'] + +This keeps the diff minimal — no new model, no new migration beyond the one +column-add, no new FK navigation in the codebase. + +## Open questions +- Should feature flags be a separate table or stay as JSONField? +- Any concerns with this approach? +`); + + run('git', ['add', '.']); + run('git', ['commit', '-m', 'add plan']); + + fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'plan-eng-review', 'SKILL.md'), + path.join(planDir, 'plan-eng-review', 'SKILL.md'), + ); + }); + + afterAll(() => { + try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {} + }); + + testConcurrentIfSelected('plan-eng-review-data-model-bias', async () => { + const result = await runSkillTest({ + prompt: `Read plan-eng-review/SKILL.md for the review workflow. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + workingDirectory: planDir, + maxTurns: 15, + timeout: 360_000, + testName: 'plan-eng-review-data-model-bias', + runId, + model: 'claude-opus-4-6', + }); + + logCost('/plan-eng-review data-model-bias', result); + + // Verify the review was written + const reviewPath = path.join(planDir, 'review-output.md'); + const review = fs.existsSync(reviewPath) + ? fs.readFileSync(reviewPath, 'utf-8') + : ''; + + // Behavioral assertions: the review should recommend a separate tier model + // and push back on JSONField for the feature set. Matching is deliberately + // loose (case-insensitive substrings) to tolerate wording variance while + // still catching the bias regression. + const lower = review.toLowerCase(); + const recommendsSeparateModel = + /separate\s+(subscription\s*)?tier\s*model/i.test(review) || + /new\s+(subscription\s*)?tier\s*model/i.test(review) || + /subscriptiontier\s*model/i.test(review) || + /extract.*tier.*model/i.test(review) || + /tier\s*(?:table|model)\s*(?:with|per)/i.test(review); + const pushesBackOnJsonField = + lower.includes('jsonfield') && + (lower.includes('feature') || lower.includes('polymorph') || lower.includes('explicit')); + const citesNormalization = + /normali[sz]/i.test(review) || + /\bsrp\b/i.test(review) || + /single\s+responsibility/i.test(review) || + /\b3nf\b/i.test(review) || + /normal\s+form/i.test(review); + + recordE2E(evalCollector, '/plan-eng-review-data-model-bias', 'Plan Eng Review Data-Model Bias E2E', result, { + passed: + ['success', 'error_max_turns'].includes(result.exitReason) && + review.length > 200 && + recommendsSeparateModel && + pushesBackOnJsonField && + citesNormalization, + }); + + expect(['success', 'error_max_turns']).toContain(result.exitReason); + expect(review.length).toBeGreaterThan(200); + expect(recommendsSeparateModel).toBe(true); + expect(pushesBackOnJsonField).toBe(true); + expect(citesNormalization).toBe(true); + }, 420_000); +}); + // --- Plan-Eng-Review Test-Plan Artifact E2E --- describeIfSelected('Plan-Eng-Review Test-Plan Artifact E2E', ['plan-eng-review-artifact'], () => { diff --git a/test/skill-validation.test.ts b/test/skill-validation.test.ts index 6515d08bb..5d603bcfc 100644 --- a/test/skill-validation.test.ts +++ b/test/skill-validation.test.ts @@ -1617,3 +1617,70 @@ describe('sidebar agent (#584)', () => { expect(content).not.toContain("proc.stderr.on('data', () => {})"); }); }); + +// Data-model bias guardrails — prevent silent deletion of the corrective bullets +// added to counter the AI's pull toward table-count minimization and JSONField +// polymorphism shortcuts. These are load-bearing: if they disappear in a future +// 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. +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'); + + test('plan-eng-review preserves the "data model exception to minimal diff" bullet', () => { + expect(engReview).toContain('Data model exception to "minimal diff"'); + expect(engReview).toContain('Count concepts, not tables'); + }); + + test('plan-eng-review preserves the JSONField polymorphism warning', () => { + expect(engReview).toContain('JSONField is not an escape hatch for polymorphism'); + 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(engReview).toContain('Data model honesty'); + expect(engReview).toContain('parent-field-shadowed-by-child'); + expect(engReview).toContain('JSONField-hiding-schema'); + }); + + test('plan-eng-review preserves the Data model review checklist subsection', () => { + expect(engReview).toContain('Data model review checklist'); + // A few load-bearing checklist items — if any disappear, the checklist was gutted + expect(engReview).toContain('Single Responsibility (SRP for models)'); + expect(engReview).toContain('Nullable with semantic meaning'); + expect(engReview).toContain('Parent-field-shadowed-by-child'); + expect(engReview).toContain('FK deletion strategies'); + expect(engReview).toContain('Snapshot vs render-live'); + }); + + test('plan-eng-review cognitive patterns list is contiguous 1–18', () => { + // If someone inserts/removes a pattern without renumbering, catch it here + for (let i = 1; i <= 18; 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 minimal diff" bullet', () => { + expect(ceoReview).toContain('Data model exception to "minimal 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'); + }); +}); From e201a7592efde48ff8d05e09d2718a578b4248f2 Mon Sep 17 00:00:00 2001 From: David Grant Date: Sat, 18 Apr 2026 12:49:18 -0700 Subject: [PATCH 02/17] chore: regenerate SKILL.md after merging main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picks up preamble resolver changes from upstream (question-tuning + writing style config echoes) that landed on main but weren't regenerated into the top-level SKILL.md yet. Pure gen-skill-docs output — no hand edits. Co-Authored-By: Claude Opus 4.7 (1M context) --- SKILL.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/SKILL.md b/SKILL.md index 70d576cdc..4d3b1d415 100644 --- a/SKILL.md +++ b/SKILL.md @@ -49,6 +49,16 @@ _TEL_START=$(date +%s) _SESSION_ID="$$-$(date +%s)" echo "TELEMETRY: ${_TEL:-off}" echo "TEL_PROMPTED: $_TEL_PROMPTED" +# Question tuning (opt-in; see /plan-tune + docs/designs/PLAN_TUNING_V0.md) +_QUESTION_TUNING=$(~/.claude/skills/gstack/bin/gstack-config get question_tuning 2>/dev/null || echo "false") +echo "QUESTION_TUNING: $_QUESTION_TUNING" +# Writing style (V1: default = ELI10-style, terse = V0 prose. See docs/designs/PLAN_TUNING_V1.md) +_EXPLAIN_LEVEL=$(~/.claude/skills/gstack/bin/gstack-config get explain_level 2>/dev/null || echo "default") +if [ "$_EXPLAIN_LEVEL" != "default" ] && [ "$_EXPLAIN_LEVEL" != "terse" ]; then _EXPLAIN_LEVEL="default"; fi +echo "EXPLAIN_LEVEL: $_EXPLAIN_LEVEL" +# V1 upgrade migration pending-prompt flag +_WRITING_STYLE_PENDING=$([ -f ~/.gstack/.writing-style-prompt-pending ] && echo "yes" || echo "no") +echo "WRITING_STYLE_PENDING: $_WRITING_STYLE_PENDING" mkdir -p ~/.gstack/analytics if [ "$_TEL" != "off" ]; then echo '{"skill":"gstack","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true @@ -110,6 +120,29 @@ of `/qa`, `/gstack-ship` instead of `/ship`). Disk paths are unaffected — alwa If output shows `UPGRADE_AVAILABLE `: read `~/.claude/skills/gstack/gstack-upgrade/SKILL.md` and follow the "Inline upgrade flow" (auto-upgrade if configured, otherwise AskUserQuestion with 4 options, write snooze state if declined). If `JUST_UPGRADED `: tell user "Running gstack v{to} (just updated!)" and continue. +If `WRITING_STYLE_PENDING` is `yes`: You're on the first skill run after upgrading +to gstack v1. Ask the user once about the new default writing style. Use AskUserQuestion: + +> v1 prompts = simpler. Technical terms get a one-sentence gloss on first use, +> questions are framed in outcome terms, sentences are shorter. +> +> Keep the new default, or prefer the older tighter prose? + +Options: +- A) Keep the new default (recommended — good writing helps everyone) +- B) Restore V0 prose — set `explain_level: terse` + +If A: leave `explain_level` unset (defaults to `default`). +If B: run `~/.claude/skills/gstack/bin/gstack-config set explain_level terse`. + +Always run (regardless of choice): +```bash +rm -f ~/.gstack/.writing-style-prompt-pending +touch ~/.gstack/.writing-style-prompted +``` + +This only happens once. If `WRITING_STYLE_PENDING` is `no`, skip this entirely. + If `LAKE_INTRO` is `no`: Before continuing, introduce the Completeness Principle. Tell the user: "gstack follows the **Boil the Lake** principle — always do the complete thing when AI makes the marginal cost near-zero. Read more: https://garryslist.org/posts/boil-the-ocean" From 63d29e4cc0ddded9ef84fe4640c78a2319381da4 Mon Sep 17 00:00:00 2001 From: David Grant Date: Sat, 18 Apr 2026 15:21:18 -0700 Subject: [PATCH 03/17] chore: bump version and changelog (v1.1.2.0) Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 13 +++++++++++++ VERSION | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e05187aa..d83ad636b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## [1.1.2.0] - 2026-04-18 + +### Changed +- **`/plan-eng-review` and `/plan-ceo-review` now defend schema normalization.** The skills' "right-sized diff" preference is sane for bug fixes and most feature work, but it was systematically pushing reviewers to merge new fields into existing models (because new tables touch more files) and to reach for JSONField as a polymorphism shortcut. Both skills now carry an explicit data-model exception: when reviewing schema changes, count concepts not tables, normalize first, denormalize only for measured reasons. JSONField gets its own explicit guidance — promote variant-specific keys to columns + CheckConstraints when the variants are knowable at design time, and reserve JSONField for the legitimate cases (third-party API caches, opaque preference bags). +- **`/plan-eng-review` Architecture Review now runs a Data Model Review Checklist.** Reviewers no longer wait for the user to push back on model shape. The new subsection runs eleven proactive checks per model touched by the plan: SRP per model, nullable-with-semantic-meaning, models-hiding-as-columns, parent-field-shadowed-by-child, JSONField-as-polymorphism-escape-hatch, FK deletion strategy, snapshot-vs-render-live values, cross-scope FK consistency, derived-state-vs-stored-state, field naming, and DB CheckConstraints over app-layer validation. +- **`/plan-eng-review` cognitive patterns gained three data-modeling instincts.** Codd's normal forms (1970), Knuth's "premature optimization" warning applied to schema, Beck's "make it right before fast," Martin's SRP applied to data models, and Structure-beats-blobs-for-known-polymorphism. The list before this change covered engineering management instincts (Larson, McKinley, Fowler, Conway, Brooks, SRE) but had nothing on data-modeling discourse — leaving the "fewer parts feels simpler" bias with no counterweight. + +### For contributors +- New regression suite in `test/skill-validation.test.ts`: ten static guardrails verify every load-bearing bullet survives future template refactors (free, sub-5ms, catches silent deletion). Cognitive-patterns list contiguity (1–18) is asserted. +- New E2E case in `test/skill-e2e-plan.test.ts` (periodic tier): a synthetic SubscriptionTier plan exercises the bias fix end-to-end. Asserts the skill recommends a separate tier model, pushes back on JSONField for variant data, and cites normalization or SRP. Runs weekly via `EVALS_TIER=periodic`. +- Caught in real `/plan-eng-review` sessions where experienced developers had to push back on the bias multiple times before landing the right shape. +- Closes garrytan/gstack#1048. + ## [1.1.1.0] - 2026-04-18 ### Fixed diff --git a/VERSION b/VERSION index 410f6a9ef..a6f417b8f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.1.1.0 +1.1.2.0 From 5374987c6cd41d2cbd95bd19a8a3d7330137f381 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 17:20:19 -0700 Subject: [PATCH 04/17] fix(plan-eng-review,plan-ceo-review): narrow absolute schema-bias language, add legitimate-JSON and measured-denormalization counterexamples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review feedback (garrytan/gstack#1071): the data-model bias guidance was overly absolute ("diff size is NOT a goal", "three clean models... in all respects", "JSONField is not an escape hatch") and lacked counterexamples for cases where JSON columns or denormalization are the right call. - "Data model exception to right-sized diff" bullet (both skills): reframes from an absolute rule to "don't let diff size alone decide the shape", explicitly allows denormalization backed by a measurement, and adds the counterexample of a field that would only ever be read through one FK join with no independent consumer. - JSONField bullet (both skills): keeps the core diagnostic but adds concrete legitimate uses — third-party webhook/OAuth payload caching, open-ended preference bags, event payloads shaped by an external producer, and schemas still being discovered. - plan-eng-review cognitive patterns #11 (normalize first) and #13 (structure beats blobs) get the same measured-reason / known-polymorphism counterexamples. - Architecture Review's "Data model honesty" bullet and the "Single Responsibility" and "JSONField as polymorphism escape hatch" checklist items (now living in sections/review-sections.md.tmpl per main's carve refactor) get matching nuance. Bumps carve-guards.ts skeleton-size budgets for plan-eng-review (69_000 -> 70_000) and plan-ceo-review (91_000 -> 92_000) to cover the added prose; verified against test/parity-suite.test.ts (13/13 pass) and test/skill-validation.test.ts's load-bearing-bullet guardrails (still pass — none of the anchor phrases the static tests check were removed). --- plan-ceo-review/SKILL.md | 4 ++-- plan-ceo-review/SKILL.md.tmpl | 4 ++-- plan-eng-review/SKILL.md | 10 +++++----- plan-eng-review/SKILL.md.tmpl | 10 +++++----- plan-eng-review/sections/review-sections.md | 10 +++++----- plan-eng-review/sections/review-sections.md.tmpl | 10 +++++----- test/helpers/carve-guards.ts | 11 +++++++---- 7 files changed, 31 insertions(+), 28 deletions(-) diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index 09b53954f..afed8f46f 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -897,8 +897,8 @@ 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, diff size is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. 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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. +* **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. diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index 916bf913a..fe7617d81 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -86,8 +86,8 @@ 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, diff size is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. 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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. +* **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. diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index c0afc6cf2..45d1f2516 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -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, diff size is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. Count concepts, not tables. The default for schema design is more clean parts, not fewer muddled ones. -* **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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. +* **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. ## Cognitive Patterns — How Great Eng Managers Think @@ -851,9 +851,9 @@ 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. The default for data model design is more clean parts, not fewer muddled ones. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast. -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). -13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time, 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. JSONField is for genuinely opaque data (third-party blobs, user preferences nobody queries on), not for "we know there are 4 kinds and each kind has different fields." +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). diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index 973946614..f66a35baa 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -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, diff size is NOT a goal.** Normalize first; denormalize only for measured performance reasons. Three clean models that each do one thing are better than one model doing three things, in all respects. When tempted to merge new fields into an existing model to reduce table count, ask: "does each model have exactly one job?" If splitting is honest, split. Count concepts, not tables. The default for schema design is more clean parts, not fewer muddled ones. -* **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), 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. Legitimate JSONField uses are genuinely schemaless data (third-party API response caches, user preference bags, opaque blobs). Polymorphism with knowable variants belongs in explicit columns + CheckConstraints. +* **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. ## Cognitive Patterns — How Great Eng Managers Think @@ -79,9 +79,9 @@ 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. The default for data model design is more clean parts, not fewer muddled ones. "Make it work, make it right, make it fast" (Beck) — right shape comes BEFORE fast. -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). -13. **Structure beats blobs for known polymorphism** — If you can enumerate the variants of a thing at design time, 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. JSONField is for genuinely opaque data (third-party blobs, user preferences nobody queries on), not for "we know there are 4 kinds and each kind has different fields." +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). diff --git a/plan-eng-review/sections/review-sections.md b/plan-eng-review/sections/review-sections.md index afd8dd2ce..7ccc56266 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -51,7 +51,7 @@ 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 at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize. Three clean models > one muddled model in all respects. +* **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? @@ -62,7 +62,7 @@ For every new model, model change, or schema migration in this plan, run through **Single Responsibility (SRP for models)** - Does this model have exactly one job? Audit ≠ balance ≠ notification ≠ display. -- If a model is doing N jobs, propose splitting it into N models. +- If a model is doing N jobs, propose splitting it into N models — unless the "extra job" is a single trivial field with no independent query pattern, write path, or consumer, in which case surface it as a tradeoff rather than mandate the split. **Nullable with semantic meaning** - For every nullable column, ask: does NULL flip what kind of thing this row is? @@ -80,9 +80,9 @@ For every new model, model change, or schema migration in this plan, run through - If yes, the parent's field belongs on the child. Remove from parent, require ≥1 child row, read via FK navigation. **JSONField as polymorphism escape hatch** -- For every JSONField, ask: am I documenting "what keys appear for which variant"? -- If yes, the JSONField has schema. Promote to explicit columns + CheckConstraints. -- Legitimate JSONField uses: third-party API caches, opaque user preferences, schemaless attribute bags nobody queries against. +- For every JSONField, ask: am I documenting "what keys appear for which variant," and are those variants stable? +- If yes to both, the JSONField has schema. Promote to explicit columns + CheckConstraints. +- Legitimate JSONField uses: caching a third-party API response verbatim (webhook payloads, OAuth profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, and early-stage schemas whose variants aren't stable yet. None of these are escape hatches — they're cases where the DB genuinely can't add value over the JSON blob. **FK deletion strategies (CASCADE / PROTECT / SET_NULL)** - For every FK, ask: if the parent is deleted, what should happen to this row? diff --git a/plan-eng-review/sections/review-sections.md.tmpl b/plan-eng-review/sections/review-sections.md.tmpl index 7bf7c84cb..65bb9b513 100644 --- a/plan-eng-review/sections/review-sections.md.tmpl +++ b/plan-eng-review/sections/review-sections.md.tmpl @@ -13,7 +13,7 @@ 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 at design time (JSONField-hiding-schema smell — promote to explicit columns + CheckConstraints)? When in doubt, normalize. Three clean models > one muddled model in all respects. +* **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? @@ -24,7 +24,7 @@ For every new model, model change, or schema migration in this plan, run through **Single Responsibility (SRP for models)** - Does this model have exactly one job? Audit ≠ balance ≠ notification ≠ display. -- If a model is doing N jobs, propose splitting it into N models. +- If a model is doing N jobs, propose splitting it into N models — unless the "extra job" is a single trivial field with no independent query pattern, write path, or consumer, in which case surface it as a tradeoff rather than mandate the split. **Nullable with semantic meaning** - For every nullable column, ask: does NULL flip what kind of thing this row is? @@ -42,9 +42,9 @@ For every new model, model change, or schema migration in this plan, run through - If yes, the parent's field belongs on the child. Remove from parent, require ≥1 child row, read via FK navigation. **JSONField as polymorphism escape hatch** -- For every JSONField, ask: am I documenting "what keys appear for which variant"? -- If yes, the JSONField has schema. Promote to explicit columns + CheckConstraints. -- Legitimate JSONField uses: third-party API caches, opaque user preferences, schemaless attribute bags nobody queries against. +- For every JSONField, ask: am I documenting "what keys appear for which variant," and are those variants stable? +- If yes to both, the JSONField has schema. Promote to explicit columns + CheckConstraints. +- Legitimate JSONField uses: caching a third-party API response verbatim (webhook payloads, OAuth profiles), a genuinely open-ended user-preferences bag nobody queries into, event/analytics payloads shaped by an external producer, and early-stage schemas whose variants aren't stable yet. None of these are escape hatches — they're cases where the DB genuinely can't add value over the JSON blob. **FK deletion strategies (CASCADE / PROTECT / SET_NULL)** - For every FK, ask: if the parent is deleted, what should happen to this row? diff --git a/test/helpers/carve-guards.ts b/test/helpers/carve-guards.ts index d49cac246..34c4f82ad 100644 --- a/test/helpers/carve-guards.ts +++ b/test/helpers/carve-guards.ts @@ -146,8 +146,9 @@ export const CARVE_GUARDS: Record = { 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 — small (~600B) but pushes past the prior 90_000 skeleton budget. - maxSkeletonBytes: 91_000, + // list, then review feedback narrowed both bullets and added legitimate-JSON / + // measured-denormalization counterexamples — pushes past the prior 90_000 budget. + maxSkeletonBytes: 92_000, minUnionBytes: 80_000, mustContain: ['SCOPE EXPANSION', 'SELECTIVE EXPANSION', 'HOLD SCOPE', 'SCOPE REDUCTION'], // Default-on Codex outside-voice (codexPreflight block + CODEX_MODE branch @@ -170,8 +171,10 @@ export const CARVE_GUARDS: Record = { // 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 — pushes past the prior 67_000 budget. - maxSkeletonBytes: 69_000, + // 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, minUnionBytes: 70_000, mustContain: ['Architecture', 'Code Quality', 'Test', 'Performance'], // Cross-cutting preamble growth (v1.57.2.0 AUQ-failure prose fallback + the From 33fbb58b13a29e89cdd0869ce98b723157ba0dbe Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 17:20:32 -0700 Subject: [PATCH 05/17] test: resolve terminology drift, rebase E2E fixture to carved templates, add positive/counterexample evals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review feedback (garrytan/gstack#1071), resolving the three Copilot review threads plus the "add positive and counterexample evaluations" ask: - test/skill-validation.test.ts: renamed the two tests still named "...to minimal diff..." to "...to right-sized diff..." to match the actual bullet text the assertions check (Copilot review comment). - test/skill-e2e-plan.test.ts: updated the Data-Model Bias regression test's header comment from "minimal diff" to "right-sized diff" (Copilot review comment); fixed the same test's fixture setup to also copy plan-eng-review's sections/ directory — main's carve refactor moved the Architecture Review and Data Model Review Checklist out of SKILL.md into sections/review-sections.md after this test was written, so the sandbox was missing the very file the skill's STOP-Read directive points at. - Added two new periodic-tier E2E cases exercising the counterexamples the templates now document: "plan-eng-review-data-model-legitimate-json" (a Stripe-webhook-style JSONField the skill should NOT push back on) and "plan-eng-review-data-model-measured-denorm" (a scoped denormalization backed by a stated APM/load-test measurement the skill should NOT insist on normalizing away). Both registered in test/helpers/touchfiles.ts (periodic tier, same classification as the existing bias regression test). Verified: bun test test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/skill-e2e-plan.test.ts — 768 pass, 39 skip (paid E2E, requires EVALS=1), 0 fail. --- test/helpers/touchfiles.ts | 4 + test/skill-e2e-plan.test.ts | 238 +++++++++++++++++++++++++++++++++- test/skill-validation.test.ts | 4 +- 3 files changed, 240 insertions(+), 6 deletions(-) diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index ac55e637a..e0590c222 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -90,6 +90,8 @@ export const E2E_TOUCHFILES: Record = { 'plan-eng-review': ['plan-eng-review/**'], 'plan-eng-review-artifact': ['plan-eng-review/**'], 'plan-eng-review-data-model-bias': ['plan-eng-review/**'], + 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**'], + 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**'], 'plan-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'], // Plan-mode smoke tests — gate-tier safety regression tests. Each test file @@ -522,6 +524,8 @@ export const E2E_TIERS: Record = { 'plan-eng-review': 'periodic', 'plan-eng-review-artifact': 'periodic', 'plan-eng-review-data-model-bias': 'periodic', + 'plan-eng-review-data-model-legitimate-json': 'periodic', + 'plan-eng-review-data-model-measured-denorm': 'periodic', 'plan-eng-coverage-audit': 'gate', 'plan-review-report': 'gate', diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 309857e04..d1935d9ba 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -371,10 +371,10 @@ Focus on architecture, code quality, tests, and performance sections.`, // Regression test for the data-model bias fix: verifies that /plan-eng-review // recommends a separate model instead of inlining columns + JSONField when // a plan tries to merge polymorphic variant data into an existing model "to -// keep the diff minimal." Before the fix, the skill's "minimal diff" preference -// pushed the AI toward accepting the inline approach; after the fix, the -// data-model exception bullet + Data model review checklist should make the -// AI push back and recommend normalization citing SRP/3NF. +// keep the diff right-sized." Before the fix, the skill's "right-sized diff" +// preference pushed the AI toward accepting the inline approach; after the +// fix, the data-model exception bullet + Data model review checklist should +// make the AI push back and recommend normalization citing SRP/3NF. describeIfSelected('Plan Eng Review Data-Model Bias E2E', ['plan-eng-review-data-model-bias'], () => { let planDir: string; @@ -432,6 +432,9 @@ column-add, no new FK navigation in the codebase. path.join(ROOT, 'plan-eng-review', 'SKILL.md'), path.join(planDir, 'plan-eng-review', 'SKILL.md'), ); + // Carved skills (v2 plan T9): copy sections/ so the Architecture review + + // Data model review checklist (now carved out of SKILL.md) are present. + { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } }); afterAll(() => { @@ -501,6 +504,233 @@ Focus specifically on the data model design in the plan. Apply the data model re }, 420_000); }); +// --- Plan Eng Review Data-Model Legitimate-JSON Counterexample E2E --- +// +// Positive-case regression: the data-model bias fix narrowed "JSONField is not +// an escape hatch for polymorphism" to explicitly exempt genuinely legitimate +// uses (third-party payload caches, opaque preference bags, schemas still +// being discovered). This verifies the narrowing actually works — that +// /plan-eng-review does NOT push back on a JSONField that squarely matches +// one of its own listed legitimate cases (caching a third-party webhook +// payload verbatim, schema controlled entirely by the external producer). + +describeIfSelected('Plan Eng Review Data-Model Legitimate JSON E2E', ['plan-eng-review-data-model-legitimate-json'], () => { + let planDir: string; + + beforeAll(() => { + planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-json-')); + const run = (cmd: string, args: string[]) => + spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); + + run('git', ['init', '-b', 'main']); + run('git', ['config', 'user.email', 'test@test.com']); + run('git', ['config', 'user.name', 'Test']); + + // Synthetic plan designed to NOT trip the JSONField guidance: the payload + // is a verbatim third-party webhook body, schema owned by Stripe (not us), + // never queried into by key — the textbook legitimate JSONField case. + fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add Stripe Webhook Event Log + +## Context +We need to store incoming Stripe webhook events for audit and replay. Stripe +sends many event types (payment_intent.succeeded, charge.refunded, +customer.subscription.updated, etc.), each with a different JSON body shape +that Stripe controls and can change without notice. + +## Proposed changes +Add a WebhookEvent model: +- id: primary key +- event_type: CharField (e.g. "payment_intent.succeeded") +- stripe_event_id: CharField, unique +- received_at: DateTimeField +- payload: JSONField — the verbatim JSON body Stripe sent, unmodified + +We will never query into specific payload keys from application code; the +only consumer is a manual replay tool that re-POSTs the raw payload to our +webhook handler for reprocessing. The field exists purely to cache Stripe's +response verbatim for audit/replay, not to model our own domain state. + +## Open questions +- Is storing the whole event as JSONField the right call here, or should we + break out the commonly-used fields into columns? +`); + + run('git', ['add', '.']); + run('git', ['commit', '-m', 'add plan']); + + fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'plan-eng-review', 'SKILL.md'), + path.join(planDir, 'plan-eng-review', 'SKILL.md'), + ); + { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } + }); + + afterAll(() => { + try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {} + }); + + testConcurrentIfSelected('plan-eng-review-data-model-legitimate-json', async () => { + const result = await runSkillTest({ + prompt: `Read plan-eng-review/SKILL.md for the review workflow. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + workingDirectory: planDir, + maxTurns: 15, + timeout: 360_000, + testName: 'plan-eng-review-data-model-legitimate-json', + runId, + model: 'claude-opus-4-6', + }); + + logCost('/plan-eng-review data-model-legitimate-json', result); + + const reviewPath = path.join(planDir, 'review-output.md'); + const review = fs.existsSync(reviewPath) + ? fs.readFileSync(reviewPath, 'utf-8') + : ''; + const lower = review.toLowerCase(); + + // Deliberately loose, case-insensitive matching (same tolerance as the + // bias-regression test above) to survive wording variance while still + // catching an over-eager JSONField warning. + const discussesThePayloadField = lower.includes('payload') || lower.includes('jsonfield'); + const pushesToSplitTheField = + /(promote|split|convert|extract|move)[^.]{0,80}payload[^.]{0,80}(column|field)/i.test(review) || + /payload[^.]{0,80}(promote|split|convert to (explicit )?column|explicit column)/i.test(review); + const acknowledgesLegitimateUse = + /legitimate|appropriate|reasonable|correct (choice|call|approach)|fine as[- ](is|it)|acceptable|(third[- ]party|external).{0,40}(payload|blob|response)|verbatim|schemaless/i.test(review); + + recordE2E(evalCollector, '/plan-eng-review-data-model-legitimate-json', 'Plan Eng Review Data-Model Legitimate JSON E2E', result, { + passed: + ['success', 'error_max_turns'].includes(result.exitReason) && + review.length > 200 && + discussesThePayloadField && + !pushesToSplitTheField && + acknowledgesLegitimateUse, + }); + + expect(['success', 'error_max_turns']).toContain(result.exitReason); + expect(review.length).toBeGreaterThan(200); + expect(discussesThePayloadField).toBe(true); + expect(pushesToSplitTheField).toBe(false); + expect(acknowledgesLegitimateUse).toBe(true); + }, 420_000); +}); + +// --- Plan Eng Review Data-Model Measured-Denormalization Counterexample E2E --- +// +// Positive-case regression: the data-model bias fix narrowed "normalize +// first" to explicitly exempt denormalization backed by a stated +// measurement (profiled hot path, load test, documented query cost). This +// verifies /plan-eng-review accepts a denormalized snapshot field when the +// plan cites a concrete measurement, instead of reflexively recommending a +// return to a fully-normalized live-join design. + +describeIfSelected('Plan Eng Review Data-Model Measured Denormalization E2E', ['plan-eng-review-data-model-measured-denorm'], () => { + let planDir: string; + + beforeAll(() => { + planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-denorm-')); + const run = (cmd: string, args: string[]) => + spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); + + run('git', ['init', '-b', 'main']); + run('git', ['config', 'user.email', 'test@test.com']); + run('git', ['config', 'user.name', 'Test']); + + // Synthetic plan designed to NOT trip the normalize-first guidance: the + // denormalization is scoped to one read path and backed by a stated + // measurement (APM p95 + load test), matching the fix's own counterexample. + fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add OrderSummary Read Snapshot for Checkout Confirmation + +## Context +The checkout confirmation page currently joins Order -> Customer -> +ShippingAddress (3 tables) to render the confirmation. APM (New Relic) shows +this read path at p95 180ms under current traffic; a load test at 2x +projected peak traffic showed p95 climbing to 340ms, driven almost entirely +by this join according to the query plan. + +## Proposed changes +Add an OrderSummary model that snapshots customer_name and +shipping_address_line onto the Order row at order-creation time, used ONLY +by the checkout confirmation read path. Order, Customer, and +ShippingAddress remain fully normalized everywhere else in the codebase — +this is a single, measured, scoped denormalization for one hot read path, +not a general schema change. + +## Open questions +- Is this the right call, or should we keep reading live via the FK chain + and instead add caching/indexing to fix the latency? +`); + + run('git', ['add', '.']); + run('git', ['commit', '-m', 'add plan']); + + fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'plan-eng-review', 'SKILL.md'), + path.join(planDir, 'plan-eng-review', 'SKILL.md'), + ); + { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } + }); + + afterAll(() => { + try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {} + }); + + testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => { + const result = await runSkillTest({ + prompt: `Read plan-eng-review/SKILL.md for the review workflow. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + workingDirectory: planDir, + maxTurns: 15, + timeout: 360_000, + testName: 'plan-eng-review-data-model-measured-denorm', + runId, + model: 'claude-opus-4-6', + }); + + logCost('/plan-eng-review data-model-measured-denorm', result); + + const reviewPath = path.join(planDir, 'review-output.md'); + const review = fs.existsSync(reviewPath) + ? fs.readFileSync(reviewPath, 'utf-8') + : ''; + const lower = review.toLowerCase(); + + const discussesTheDenormalization = lower.includes('denormal') || lower.includes('ordersummary'); + const rejectsAsUnjustified = + /(premature|not[- ]yet justified|lacks?[- ]a measurement|no measurement|remove[^.]{0,40}(denormali[sz]ation|snapshot)|normali[sz]e[^.]{0,40}(instead|back|it)|should (read|query|join)[^.]{0,40}live)/i.test(review); + const acceptsMeasuredJustification = + /measur|profil|load test|p95|latency|query plan|justified|reasonable|legitimate|appropriate|scoped/i.test(review); + + recordE2E(evalCollector, '/plan-eng-review-data-model-measured-denorm', 'Plan Eng Review Data-Model Measured Denormalization E2E', result, { + passed: + ['success', 'error_max_turns'].includes(result.exitReason) && + review.length > 200 && + discussesTheDenormalization && + !rejectsAsUnjustified && + acceptsMeasuredJustification, + }); + + expect(['success', 'error_max_turns']).toContain(result.exitReason); + expect(review.length).toBeGreaterThan(200); + expect(discussesTheDenormalization).toBe(true); + expect(rejectsAsUnjustified).toBe(false); + expect(acceptsMeasuredJustification).toBe(true); + }, 420_000); +}); + // --- Plan-Eng-Review Test-Plan Artifact E2E --- describeIfSelected('Plan-Eng-Review Test-Plan Artifact E2E', ['plan-eng-review-artifact'], () => { diff --git a/test/skill-validation.test.ts b/test/skill-validation.test.ts index 5361ce9c0..3e0c0246c 100644 --- a/test/skill-validation.test.ts +++ b/test/skill-validation.test.ts @@ -1994,7 +1994,7 @@ describe('data-model bias guardrails', () => { const engReviewSections = fs.readFileSync( path.join(ROOT, 'plan-eng-review', 'sections', 'review-sections.md'), 'utf-8'); - test('plan-eng-review preserves the "data model exception to minimal diff" bullet', () => { + 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'); }); @@ -2041,7 +2041,7 @@ describe('data-model bias guardrails', () => { expect(engReview).not.toMatch(/^19\. \*\*/m); }); - test('plan-ceo-review preserves the "data model exception to minimal diff" bullet', () => { + 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'); }); From d7567453c85398b616117c0520f70a8e7f0ba84e Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 17:20:42 -0700 Subject: [PATCH 06/17] chore: drop mid-branch VERSION/CHANGELOG bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review feedback (garrytan/gstack#1071) flagged this as release metadata that doesn't belong in a feature PR, and Copilot separately flagged that the bump had already drifted (VERSION at 1.1.2.0, package.json still at 1.1.1.0). Per this repo's own CHANGELOG convention, the version bump and changelog entry belong at /ship time — covering every commit that actually landed on the branch — not mid-review while the content is still being revised. Reverts VERSION and package.json to match main's current 1.60.1.0 and drops the premature CHANGELOG entry; /ship will add the real one once this branch is ready to merge. --- CHANGELOG.md | 13 ------------- VERSION | 2 +- package.json | 2 +- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27f63c1da..351e7cabd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,18 +1,5 @@ # Changelog -## [1.60.2.0] - 2026-07-15 - -### Changed -- **`/plan-eng-review` and `/plan-ceo-review` now defend schema normalization.** The skills' "right-sized diff" preference is sane for bug fixes and most feature work, but it was systematically pushing reviewers to merge new fields into existing models (because new tables touch more files) and to reach for JSONField as a polymorphism shortcut. Both skills now carry an explicit data-model exception: when reviewing schema changes, count concepts not tables, normalize first, denormalize only for measured reasons. JSONField gets its own explicit guidance — promote variant-specific keys to columns + CheckConstraints when the variants are knowable at design time, and reserve JSONField for the legitimate cases (third-party API caches, opaque preference bags). -- **`/plan-eng-review` Architecture Review now runs a Data Model Review Checklist.** Reviewers no longer wait for the user to push back on model shape. The new subsection runs eleven proactive checks per model touched by the plan: SRP per model, nullable-with-semantic-meaning, models-hiding-as-columns, parent-field-shadowed-by-child, JSONField-as-polymorphism-escape-hatch, FK deletion strategy, snapshot-vs-render-live values, cross-scope FK consistency, derived-state-vs-stored-state, field naming, and DB CheckConstraints over app-layer validation. -- **`/plan-eng-review` cognitive patterns gained three data-modeling instincts.** Codd's normal forms (1970), Knuth's "premature optimization" warning applied to schema, Beck's "make it right before fast," Martin's SRP applied to data models, and Structure-beats-blobs-for-known-polymorphism. The list before this change covered engineering management instincts (Larson, McKinley, Fowler, Conway, Brooks, SRE) but had nothing on data-modeling discourse — leaving the "fewer parts feels simpler" bias with no counterweight. - -### For contributors -- New regression suite in `test/skill-validation.test.ts`: ten static guardrails verify every load-bearing bullet survives future template refactors (free, sub-5ms, catches silent deletion). Cognitive-patterns list contiguity (1–18) is asserted. -- New E2E case in `test/skill-e2e-plan.test.ts` (periodic tier): a synthetic SubscriptionTier plan exercises the bias fix end-to-end. Asserts the skill recommends a separate tier model, pushes back on JSONField for variant data, and cites normalization or SRP. Runs weekly via `EVALS_TIER=periodic`. -- Caught in real `/plan-eng-review` sessions where experienced developers had to push back on the bias multiple times before landing the right shape. -- Closes garrytan/gstack#1048. - ## [1.60.1.0] - 2026-07-09 ## **The /autoplan dual-voice eval is back on the board, catching real regressions.** diff --git a/VERSION b/VERSION index 762f17554..c4190e004 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.60.2.0 +1.60.1.0 diff --git a/package.json b/package.json index 78b98a86f..846438a60 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.60.2.0", + "version": "1.60.1.0", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", From 09ad6fb1ef917aa51c42c065efd137556cd32128 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 17:37:23 -0700 Subject: [PATCH 07/17] fix: harden new data-model eval regexes, dedupe E2E fixture setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ran /review on this branch's diff against main. The always-on testing specialist found real correctness bugs in the two eval blocks added in 33fbb58b: - pushesToSplitTheField (legitimate-JSON eval) was negation-blind: a CORRECT "I would NOT extract the payload into columns" answer contains the same verb+payload+column proximity as an incorrect one, so it could fail the test on a passing review. Also missing "normalize" from its verb list, so a real regression phrased as "normalize this payload into columns" would slip through undetected. - rejectsAsUnjustified (measured-denorm eval) missed common rejection phrasings ("instead of denormalizing", "rather than snapshotting"), and its normali[sz]e-instead/back/it clause was loose enough to false-positive on approving language ("stays normalized elsewhere; keep it that way"). - recommendsSeparateModel (pre-existing data-model-bias eval, touched by this branch to fix its fixture) broke on backtick-wrapped model names, a common LLM markdown convention. Added matchesUnnegated() to test/helpers/e2e-helpers.ts — walks every regex match and checks the ~20 chars before it for a negation word before counting it as a positive signal — and used it for the split-detector. Broadened the rejection regex and tightened its false-positive-prone clause. Stripped markdown formatting before the backtick-affected check. Verified all four fixes against representative sentences (correct-negative, actual-positive, previously-missed-regression, and previously-false-positive cases) before committing — all passed as expected. Also extracted the maintainability specialist's DRY finding: the three data-model E2E blocks (bias, legitimate-json, measured-denorm) each duplicated the same ~20-line mkdtemp/git-init/write-plan/copy-skill fixture setup. Extracted into setupPlanEngReviewFixture() in e2e-helpers.ts. bun test test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts test/skill-e2e-plan.test.ts — 768 pass, 39 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.ts | 52 ++++++++++++++++++++ test/skill-e2e-plan.test.ts | 98 +++++++++++-------------------------- 2 files changed, 81 insertions(+), 69 deletions(-) diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index 32510f13a..9b8cb93d6 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -102,6 +102,58 @@ export function copyDirSync(src: string, dest: string) { } } +/** + * Set up a throwaway git repo containing plan.md + a copy of plan-eng-review's + * SKILL.md (+ sections/ if the skill has been carved) for standalone-plan E2E + * tests. Shared by the data-model-bias / legitimate-JSON / measured-denorm + * regression tests, which otherwise duplicated this ~20-line setup verbatim. + */ +export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: string): string { + const planDir = fs.mkdtempSync(path.join(os.tmpdir(), tmpPrefix)); + const run = (cmd: string, args: string[]) => + spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); + + run('git', ['init', '-b', 'main']); + run('git', ['config', 'user.email', 'test@test.com']); + run('git', ['config', 'user.name', 'Test']); + + fs.writeFileSync(path.join(planDir, 'plan.md'), planMarkdown); + run('git', ['add', '.']); + run('git', ['commit', '-m', 'add plan']); + + fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'plan-eng-review', 'SKILL.md'), + path.join(planDir, 'plan-eng-review', 'SKILL.md'), + ); + const sectionsDir = path.join(ROOT, 'plan-eng-review', 'sections'); + if (fs.existsSync(sectionsDir)) { + fs.cpSync(sectionsDir, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); + } + return planDir; +} + +/** + * Case-insensitive check for `pattern` in `text` that ignores matches + * immediately preceded by a negation word (not/n't/no/never/without/...). + * Plain substring/regex matching can't tell "extract the payload into + * columns" from "would NOT extract the payload into columns" — this walks + * every match and inspects the text just before it for a negation cue. + */ +export function matchesUnnegated(text: string, pattern: RegExp): boolean { + const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against)\b/i; + const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; + const re = new RegExp(pattern.source, flags); + let m: RegExpExecArray | null; + while ((m = re.exec(text))) { + const windowStart = Math.max(0, m.index - 20); + const preceding = text.slice(windowStart, m.index); + if (!NEGATION.test(preceding)) return true; + if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches + } + return false; +} + /** * Set up browse shims (binary symlink, find-browse, remote-slug) in a tmpDir. */ diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index d1935d9ba..59726ee02 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -5,6 +5,7 @@ import { describeIfSelected, testConcurrentIfSelected, copyDirSync, setupBrowseShims, logCost, recordE2E, createEvalCollector, finalizeEvalCollector, + setupPlanEngReviewFixture, matchesUnnegated, } from './helpers/e2e-helpers'; import { judgePosture } from './helpers/llm-judge'; import { spawnSync } from 'child_process'; @@ -380,20 +381,12 @@ describeIfSelected('Plan Eng Review Data-Model Bias E2E', ['plan-eng-review-data let planDir: string; beforeAll(() => { - planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-dmb-')); - const run = (cmd: string, args: string[]) => - spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); - - run('git', ['init', '-b', 'main']); - run('git', ['config', 'user.email', 'test@test.com']); - run('git', ['config', 'user.name', 'Test']); - // Synthetic plan designed to trip the old bias: four clearly-polymorphic // tier variants + feature-flag bag that the user proposes to inline onto // User rather than create a new model. After the fix, the skill should // recommend a separate SubscriptionTier model and push back on the // JSONField for tier_features. - fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add Subscription Tiers to User Model + planDir = setupPlanEngReviewFixture('skill-e2e-plan-eng-dmb-', `# Plan: Add Subscription Tiers to User Model ## Context I want to add a 'subscription tier' feature to my Django app. Each user can @@ -423,18 +416,6 @@ column-add, no new FK navigation in the codebase. - Should feature flags be a separate table or stay as JSONField? - Any concerns with this approach? `); - - run('git', ['add', '.']); - run('git', ['commit', '-m', 'add plan']); - - fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); - fs.copyFileSync( - path.join(ROOT, 'plan-eng-review', 'SKILL.md'), - path.join(planDir, 'plan-eng-review', 'SKILL.md'), - ); - // Carved skills (v2 plan T9): copy sections/ so the Architecture review + - // Data model review checklist (now carved out of SKILL.md) are present. - { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } }); afterAll(() => { @@ -469,14 +450,18 @@ Focus specifically on the data model design in the plan. Apply the data model re // Behavioral assertions: the review should recommend a separate tier model // and push back on JSONField for the feature set. Matching is deliberately // loose (case-insensitive substrings) to tolerate wording variance while - // still catching the bias regression. + // still catching the bias regression. Strip markdown formatting chars + // (backticks, asterisks) first — LLMs commonly wrap identifiers like + // `SubscriptionTier` in backticks, which would otherwise break the \s* + // assumptions below. const lower = review.toLowerCase(); + const unformatted = review.replace(/[`*_]/g, ''); const recommendsSeparateModel = - /separate\s+(subscription\s*)?tier\s*model/i.test(review) || - /new\s+(subscription\s*)?tier\s*model/i.test(review) || - /subscriptiontier\s*model/i.test(review) || - /extract.*tier.*model/i.test(review) || - /tier\s*(?:table|model)\s*(?:with|per)/i.test(review); + /separate\s+(subscription\s*)?tier\s*model/i.test(unformatted) || + /new\s+(subscription\s*)?tier\s*model/i.test(unformatted) || + /subscriptiontier\s*model/i.test(unformatted) || + /extract[\s\S]*tier[\s\S]*model/i.test(unformatted) || + /tier\s*(?:table|model)\s*(?:with|per)/i.test(unformatted); const pushesBackOnJsonField = lower.includes('jsonfield') && (lower.includes('feature') || lower.includes('polymorph') || lower.includes('explicit')); @@ -518,18 +503,10 @@ describeIfSelected('Plan Eng Review Data-Model Legitimate JSON E2E', ['plan-eng- let planDir: string; beforeAll(() => { - planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-json-')); - const run = (cmd: string, args: string[]) => - spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); - - run('git', ['init', '-b', 'main']); - run('git', ['config', 'user.email', 'test@test.com']); - run('git', ['config', 'user.name', 'Test']); - // Synthetic plan designed to NOT trip the JSONField guidance: the payload // is a verbatim third-party webhook body, schema owned by Stripe (not us), // never queried into by key — the textbook legitimate JSONField case. - fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add Stripe Webhook Event Log + planDir = setupPlanEngReviewFixture('skill-e2e-plan-eng-json-', `# Plan: Add Stripe Webhook Event Log ## Context We need to store incoming Stripe webhook events for audit and replay. Stripe @@ -554,16 +531,6 @@ response verbatim for audit/replay, not to model our own domain state. - Is storing the whole event as JSONField the right call here, or should we break out the commonly-used fields into columns? `); - - run('git', ['add', '.']); - run('git', ['commit', '-m', 'add plan']); - - fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); - fs.copyFileSync( - path.join(ROOT, 'plan-eng-review', 'SKILL.md'), - path.join(planDir, 'plan-eng-review', 'SKILL.md'), - ); - { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } }); afterAll(() => { @@ -597,11 +564,16 @@ Focus specifically on the data model design in the plan. Apply the data model re // Deliberately loose, case-insensitive matching (same tolerance as the // bias-regression test above) to survive wording variance while still - // catching an over-eager JSONField warning. + // catching an over-eager JSONField warning. matchesUnnegated() ignores + // matches preceded by a negation word — otherwise a CORRECT "I would NOT + // extract the payload into columns" answer would trip this check, since + // it contains "extract...payload...column" just like an incorrect one. + // Includes normali[sz]e alongside promote/split/convert/extract/move since + // that's the exact vocabulary the companion template edits use. const discussesThePayloadField = lower.includes('payload') || lower.includes('jsonfield'); const pushesToSplitTheField = - /(promote|split|convert|extract|move)[^.]{0,80}payload[^.]{0,80}(column|field)/i.test(review) || - /payload[^.]{0,80}(promote|split|convert to (explicit )?column|explicit column)/i.test(review); + matchesUnnegated(review, /(promote|split|convert|extract|move|normali[sz]e)[^.]{0,80}payload[^.]{0,80}(column|field)/i) || + matchesUnnegated(review, /payload[^.]{0,80}(promote|split|convert to (explicit )?column|explicit column|normali[sz]e)/i); const acknowledgesLegitimateUse = /legitimate|appropriate|reasonable|correct (choice|call|approach)|fine as[- ](is|it)|acceptable|(third[- ]party|external).{0,40}(payload|blob|response)|verbatim|schemaless/i.test(review); @@ -635,18 +607,10 @@ describeIfSelected('Plan Eng Review Data-Model Measured Denormalization E2E', [' let planDir: string; beforeAll(() => { - planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-eng-denorm-')); - const run = (cmd: string, args: string[]) => - spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 }); - - run('git', ['init', '-b', 'main']); - run('git', ['config', 'user.email', 'test@test.com']); - run('git', ['config', 'user.name', 'Test']); - // Synthetic plan designed to NOT trip the normalize-first guidance: the // denormalization is scoped to one read path and backed by a stated // measurement (APM p95 + load test), matching the fix's own counterexample. - fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add OrderSummary Read Snapshot for Checkout Confirmation + planDir = setupPlanEngReviewFixture('skill-e2e-plan-eng-denorm-', `# Plan: Add OrderSummary Read Snapshot for Checkout Confirmation ## Context The checkout confirmation page currently joins Order -> Customer -> @@ -667,16 +631,6 @@ not a general schema change. - Is this the right call, or should we keep reading live via the FK chain and instead add caching/indexing to fix the latency? `); - - run('git', ['add', '.']); - run('git', ['commit', '-m', 'add plan']); - - fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true }); - fs.copyFileSync( - path.join(ROOT, 'plan-eng-review', 'SKILL.md'), - path.join(planDir, 'plan-eng-review', 'SKILL.md'), - ); - { const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); } }); afterAll(() => { @@ -709,8 +663,14 @@ Focus specifically on the data model design in the plan. Apply the data model re const lower = review.toLowerCase(); const discussesTheDenormalization = lower.includes('denormal') || lower.includes('ordersummary'); + // Broadened per review feedback: the original pattern missed common + // rejection phrasings ("instead of denormalizing", "cache/index instead") + // and its "normali[sz]e...instead|back|it" clause was loose enough to + // false-positive on approving language ("stays normalized elsewhere; + // keep it that way") — tightened to require an explicit join/read/query + // + instead framing. const rejectsAsUnjustified = - /(premature|not[- ]yet justified|lacks?[- ]a measurement|no measurement|remove[^.]{0,40}(denormali[sz]ation|snapshot)|normali[sz]e[^.]{0,40}(instead|back|it)|should (read|query|join)[^.]{0,40}live)/i.test(review); + /(premature|not[- ]yet justified|lacks?[- ]a measurement|no measurement|remove[^.]{0,40}(denormali[sz]ation|snapshot)|normali[sz]e[^.]{0,20}(this|the)?[^.]{0,20}(join|read|query)[^.]{0,20}instead|instead of denormali[sz]ing|rather than (denormali[sz]ing|snapshot(ting)?)|(cache|index)[^.]{0,30}instead|should (read|query|join)[^.]{0,40}live)/i.test(review); const acceptsMeasuredJustification = /measur|profil|load test|p95|latency|query plan|justified|reasonable|legitimate|appropriate|scoped/i.test(review); From a9124bd15b9246015d4269c2eb73ab80129134d1 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:08:11 -0700 Subject: [PATCH 08/17] test: add minimal-change counterexample eval, unit-test matchesUnnegated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit garrytan/gstack#1048 comment asked for concrete evaluations covering three cases: normalized models, justified JSON fields, and minimal-change cases. The prior commits covered the first two (data-model-bias, legitimate-json, measured-denorm) but not the third — a plan where keeping a single trivial field inline (not extracting a new model) is the correct call. This was independently flagged by the ship-workflow coverage audit as the highest- severity gap in this branch's test coverage. Adds plan-eng-review-data-model-minimal-change: a synthetic plan adding one nullable timestamp field to an existing model, no polymorphism, no JSON, no independent query pattern or consumer. Asserts the skill does NOT reflexively recommend extracting a separate model for it — exercising the "unless the extra job is a single trivial field..." exception added to cognitive pattern #12 and the SRP checklist item in 5374987c. Also adds test/helpers/e2e-helpers.test.ts: free, deterministic unit tests for matchesUnnegated() and setupPlanEngReviewFixture(), which were previously only exercised indirectly by the paid, EVALS=1-gated E2E tests (also flagged by the coverage audit — a bug in either could silently flip an eval's pass/fail verdict and look like ordinary LLM wording variance). Along the way, found and documented a real limitation: the negation-window check correctly handles multiple sentences (period-bounded), but a single comma-spliced sentence containing both a negated and a positive match can still be absorbed into one greedy match — accepted as a known edge case, since real review prose reliably separates points with periods/bullets. bun test test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/skill-e2e-plan.test.ts test/helpers/e2e-helpers.test.ts test/gen-skill-docs.test.ts — all pass, 0 fail. --- test/helpers/e2e-helpers.test.ts | 102 +++++++++++++++++++++++++++++++ test/helpers/touchfiles.ts | 2 + test/skill-e2e-plan.test.ts | 95 ++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+) create mode 100644 test/helpers/e2e-helpers.test.ts diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts new file mode 100644 index 000000000..5e61561ea --- /dev/null +++ b/test/helpers/e2e-helpers.test.ts @@ -0,0 +1,102 @@ +/** + * Free, deterministic unit tests for the pure/local helpers added alongside + * the schema-consolidation-bias E2E evals. matchesUnnegated() and + * setupPlanEngReviewFixture() were previously only exercised indirectly by + * the paid, EVALS=1-gated E2E tests in skill-e2e-plan.test.ts — a bug in + * either could silently flip an eval's pass/fail verdict and be + * indistinguishable from ordinary LLM wording variance. These tests run in + * the free `bun test` suite instead. + */ + +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import { matchesUnnegated, setupPlanEngReviewFixture, ROOT } from './e2e-helpers'; +import * as path from 'path'; + +describe('matchesUnnegated', () => { + const splitPattern = /(promote|split|convert|extract|move|normali[sz]e)[^.]{0,80}payload[^.]{0,80}(column|field)/i; + + test('finds an unnegated positive match', () => { + expect(matchesUnnegated( + 'I recommend you promote the payload into explicit columns for the common fields.', + splitPattern, + )).toBe(true); + }); + + test('ignores a match preceded by "not"', () => { + expect(matchesUnnegated( + 'I would not extract the payload into columns, since the schema is Stripe-controlled.', + splitPattern, + )).toBe(false); + }); + + test('ignores a match preceded by "n\'t" (doesn\'t / wouldn\'t / shouldn\'t)', () => { + expect(matchesUnnegated( + 'This wouldn\'t promote the payload into columns.', + splitPattern, + )).toBe(false); + }); + + test('ignores a match preceded by "without"', () => { + expect(matchesUnnegated( + 'The payload stays as-is without splitting the payload into columns.', + splitPattern, + )).toBe(false); + }); + + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { + expect(matchesUnnegated( + 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', + splitPattern, + )).toBe(true); + }); + + test('finds a positive match after a negated one in an earlier sentence (does not short-circuit on the first match)', () => { + // First sentence is negated ("not extract..."), second is a genuine + // recommendation ("should promote..."). The period between them bounds + // the pattern's [^.]{0,80} window to one sentence each, so this must + // find two distinct matches — matchesUnnegated must not stop scanning + // after the first (negated) hit. + expect(matchesUnnegated( + 'I would not extract the payload into columns for the rare fields. For the common fields, I would promote the payload into explicit columns.', + splitPattern, + )).toBe(true); + }); + + test('returns false when the pattern never matches at all', () => { + expect(matchesUnnegated('This review has nothing to do with payloads or columns.', splitPattern)).toBe(false); + }); + + test('does not infinite-loop on a zero-width-adjacent pattern', () => { + // Guards the re.lastIndex++ zero-width-match protection. + const zeroWidthish = /x*/i; + expect(() => matchesUnnegated('no x here', zeroWidthish)).not.toThrow(); + }); + + test('accepts a pattern that already has the global flag', () => { + const globalPattern = /payload/gi; + expect(matchesUnnegated('the payload arrived', globalPattern)).toBe(true); + }); +}); + +describe('setupPlanEngReviewFixture', () => { + test('creates a git repo with plan.md, SKILL.md, and sections/ copied from the real skill', () => { + const planDir = setupPlanEngReviewFixture('e2e-helpers-test-fixture-', '# Plan: test fixture\n\nSome content.\n'); + try { + expect(fs.existsSync(path.join(planDir, '.git'))).toBe(true); + expect(fs.readFileSync(path.join(planDir, 'plan.md'), 'utf-8')).toContain('Plan: test fixture'); + expect(fs.existsSync(path.join(planDir, 'plan-eng-review', 'SKILL.md'))).toBe(true); + + const realSkillMd = fs.readFileSync(path.join(ROOT, 'plan-eng-review', 'SKILL.md'), 'utf-8'); + const copiedSkillMd = fs.readFileSync(path.join(planDir, 'plan-eng-review', 'SKILL.md'), 'utf-8'); + expect(copiedSkillMd).toBe(realSkillMd); + + const realSectionsDir = path.join(ROOT, 'plan-eng-review', 'sections'); + if (fs.existsSync(realSectionsDir)) { + expect(fs.existsSync(path.join(planDir, 'plan-eng-review', 'sections', 'review-sections.md'))).toBe(true); + } + } finally { + fs.rmSync(planDir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index e0590c222..ea58bf4ab 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -92,6 +92,7 @@ export const E2E_TOUCHFILES: Record = { 'plan-eng-review-data-model-bias': ['plan-eng-review/**'], 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**'], 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**'], + 'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**'], 'plan-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'], // Plan-mode smoke tests — gate-tier safety regression tests. Each test file @@ -526,6 +527,7 @@ export const E2E_TIERS: Record = { 'plan-eng-review-data-model-bias': 'periodic', 'plan-eng-review-data-model-legitimate-json': 'periodic', 'plan-eng-review-data-model-measured-denorm': 'periodic', + 'plan-eng-review-data-model-minimal-change': 'periodic', 'plan-eng-coverage-audit': 'gate', 'plan-review-report': 'gate', diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 59726ee02..9bee7c542 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -691,6 +691,101 @@ Focus specifically on the data model design in the plan. Apply the data model re }, 420_000); }); +// --- Plan Eng Review Data-Model Minimal-Change Counterexample E2E --- +// +// Positive-case regression: the data-model bias fix narrowed the SRP-for-models +// checklist item and cognitive pattern #12 to exempt a single trivial field +// with no independent query pattern, write path, or consumer — that case is +// a genuine judgment call to keep inline, not an automatic split. This +// verifies /plan-eng-review does NOT reflexively recommend extracting a new +// model for a plan that adds exactly one such field to an existing model. + +describeIfSelected('Plan Eng Review Data-Model Minimal Change E2E', ['plan-eng-review-data-model-minimal-change'], () => { + let planDir: string; + + beforeAll(() => { + // Synthetic plan designed to NOT trip the SRP/normalize guidance: one + // trivial timestamp field, no polymorphism, no JSONField, no independent + // query pattern or consumer — the textbook "keep it inline" case. + planDir = setupPlanEngReviewFixture('skill-e2e-plan-eng-minimal-', `# Plan: Add Email Verification Timestamp to User Model + +## Context +We want to show "Verified on {date}" in a single admin-dashboard column for +each user. Nothing else in the codebase reads this value — there's no +separate verification workflow, no independent lifecycle, and no other +consumer planned. + +## Proposed changes +Add one field to the existing User model: +- email_verified_at: DateTimeField, nullable (null = not yet verified) + +No new model, no JSONField, no polymorphism — one column on User, set once +when the verification email link is clicked. + +## Open questions +- Is a single column the right call here, or should this become a separate + EmailVerification model? +`); + }); + + afterAll(() => { + try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {} + }); + + testConcurrentIfSelected('plan-eng-review-data-model-minimal-change', async () => { + const result = await runSkillTest({ + prompt: `Read plan-eng-review/SKILL.md for the review workflow. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + workingDirectory: planDir, + maxTurns: 15, + timeout: 360_000, + testName: 'plan-eng-review-data-model-minimal-change', + runId, + model: 'claude-opus-4-6', + }); + + logCost('/plan-eng-review data-model-minimal-change', result); + + const reviewPath = path.join(planDir, 'review-output.md'); + const review = fs.existsSync(reviewPath) + ? fs.readFileSync(reviewPath, 'utf-8') + : ''; + const lower = review.toLowerCase(); + + const discussesTheField = lower.includes('email_verified_at') || lower.includes('verification'); + // matchesUnnegated so a CORRECT "I would NOT extract a separate model + // here" answer doesn't trip this check just for containing the words. + const recommendsSeparateModel = matchesUnnegated( + review, + /(extract|split|promote|create)[^.]{0,60}(separate|new|dedicated)[^.]{0,40}(model|table)/i, + ) || matchesUnnegated(review, /separate\s+email\s*verification\s*model/i); + const acceptsInlineAddition = + /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i.test(review) || + /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i.test(review) || + /doesn'?t (need|require|warrant)[^.]{0,40}(a\s+)?(separate|new|dedicated)[^.]{0,40}(model|table)/i.test(review); + + recordE2E(evalCollector, '/plan-eng-review-data-model-minimal-change', 'Plan Eng Review Data-Model Minimal Change E2E', result, { + passed: + ['success', 'error_max_turns'].includes(result.exitReason) && + review.length > 200 && + discussesTheField && + !recommendsSeparateModel && + acceptsInlineAddition, + }); + + expect(['success', 'error_max_turns']).toContain(result.exitReason); + expect(review.length).toBeGreaterThan(200); + expect(discussesTheField).toBe(true); + expect(recommendsSeparateModel).toBe(false); + expect(acceptsInlineAddition).toBe(true); + }, 420_000); +}); + // --- Plan-Eng-Review Test-Plan Artifact E2E --- describeIfSelected('Plan-Eng-Review Test-Plan Artifact E2E', ['plan-eng-review-artifact'], () => { From aa49339b27e02a9b9a7ee90fd683e2017b38cb22 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:16:50 -0700 Subject: [PATCH 09/17] fix: widen matchesUnnegated to sentence boundary, catch contrastive phrasing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship-workflow review re-dispatched the testing specialist against the full updated diff (including the new minimal-change eval) and found 3 more real bugs in the same family as the earlier fix pass: - matchesUnnegated's fixed 20-char negation lookback was too narrow: the patterns it guards have their own internal gaps up to 80 chars, so "I do not think it's worth extracting the payload into columns" (negation >20 chars before the match) would be misread as an unnegated recommendation. Now scans back to the start of the current sentence (last ./!/? before the match) instead of a fixed count — matches the same period-bounded assumption the calling patterns already make. - Verified during the fix: neither "not"/"never"/etc. nor the wider window catches contrastive phrasing ("add this field to the model RATHER THAN create a separate table") — the rejected alternative matches the pattern just as strongly as a genuine recommendation, with no negation word anywhere nearby. Added "rather than"/"instead of" to the shared negation list, benefiting all four callers, not just the new eval. - The new minimal-change eval's acceptsInlineAddition check was missing the matchesUnnegated guard entirely on its first alternative (unlike its sibling recommendsSeparateModel a few lines above), and recommendsSeparateModel's verb list missed common recommendation phrasings (introduce, build, spin out, break out) — deliberately did NOT add "add", since "add this new field to the existing model" is exactly how the CORRECT answer gets phrased. Added 3 unit tests reproducing the sentence-boundary and contrastive-phrasing fixes directly (not just via the paid E2E path). bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — 781 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 35 +++++++++++++++++++++++++++++++- test/helpers/e2e-helpers.ts | 25 ++++++++++++++++++++--- test/skill-e2e-plan.test.ts | 9 +++++--- 3 files changed, 62 insertions(+), 7 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index 5e61561ea..47701a6e9 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -44,6 +44,24 @@ describe('matchesUnnegated', () => { )).toBe(false); }); + test('ignores a match that is the rejected half of a "rather than" contrast', () => { + // "Keep it on the existing model RATHER THAN promote the payload into + // columns" — the rejected alternative matches the pattern just as + // strongly as a real recommendation would, with no single negation + // word (not/never/without/...) anywhere near it. + expect(matchesUnnegated( + 'Keep this field on the existing model rather than promote the payload into explicit columns.', + splitPattern, + )).toBe(false); + }); + + test('ignores a match that is the rejected half of an "instead of" contrast', () => { + expect(matchesUnnegated( + 'Keep this on the existing model instead of splitting the payload into columns.', + splitPattern, + )).toBe(false); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', @@ -67,8 +85,23 @@ describe('matchesUnnegated', () => { expect(matchesUnnegated('This review has nothing to do with payloads or columns.', splitPattern)).toBe(false); }); + test('ignores a negation far earlier in the same (period-free) sentence', () => { + // The negation-lookback scans to the sentence boundary, not a fixed + // character count — this is the scenario that motivated that design. + // "not" sits well over 20 characters before "extract...payload...columns" + // but is still part of the same sentence (no period between them). + expect(matchesUnnegated( + "I do not think it's worth the added complexity of maintaining a separate table just to extract the payload into columns for these rare cases.", + splitPattern, + )).toBe(false); + }); + test('does not infinite-loop on a zero-width-adjacent pattern', () => { - // Guards the re.lastIndex++ zero-width-match protection. + // Guards the re.lastIndex++ zero-width-match protection. Note: a bare + // quantifier pattern like /x*/ always produces an empty match at index 0 + // with nothing preceding it (so it's trivially "unnegated" and returns + // true immediately) — this test's value is that it terminates at all, + // not that it exercises the negated branch of the loop. const zeroWidthish = /x*/i; expect(() => matchesUnnegated('no x here', zeroWidthish)).not.toThrow(); }); diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index 9b8cb93d6..e09e6b352 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -139,15 +139,34 @@ export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: strin * Plain substring/regex matching can't tell "extract the payload into * columns" from "would NOT extract the payload into columns" — this walks * every match and inspects the text just before it for a negation cue. + * + * The negation lookback scans back to the start of the CURRENT SENTENCE + * (the last `.`/`!`/`?` before the match) rather than a fixed character + * count. A fixed window (originally 20 chars) is too narrow: the patterns + * this is used with have their own internal gaps of up to 80 chars (e.g. + * `(verb)[^.]{0,80}payload[^.]{0,80}column`), so a negation word like "not" + * can legitimately sit further back in the same sentence than a small fixed + * window would see — "I do not think it's worth extracting the payload into + * columns" would otherwise be misread as an unnegated recommendation. + * Scanning to the sentence boundary matches the same period-bounded + * assumption the calling patterns already make. */ export function matchesUnnegated(text: string, pattern: RegExp): boolean { - const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against)\b/i; + // "rather than"/"instead of" catch contrastive phrasing ("add this field + // to the model RATHER THAN create a separate table") — the rejected + // alternative can otherwise match the pattern just as strongly as a real + // recommendation would, with no single negation word anywhere near it. + const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against|rather than|instead of)\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); let m: RegExpExecArray | null; while ((m = re.exec(text))) { - const windowStart = Math.max(0, m.index - 20); - const preceding = text.slice(windowStart, m.index); + const sentenceStart = Math.max( + text.lastIndexOf('.', m.index), + text.lastIndexOf('!', m.index), + text.lastIndexOf('?', m.index), + ) + 1; + const preceding = text.slice(sentenceStart, m.index); if (!NEGATION.test(preceding)) return true; if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches } diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 9bee7c542..1a4dbb7f7 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -760,13 +760,16 @@ Focus specifically on the data model design in the plan. Apply the data model re const discussesTheField = lower.includes('email_verified_at') || lower.includes('verification'); // matchesUnnegated so a CORRECT "I would NOT extract a separate model // here" answer doesn't trip this check just for containing the words. + // Verb list deliberately excludes "add" — "add this new field to the + // existing model" is exactly how a CORRECT (inline) recommendation gets + // phrased, so including it would false-positive on the desired answer. const recommendsSeparateModel = matchesUnnegated( review, - /(extract|split|promote|create)[^.]{0,60}(separate|new|dedicated)[^.]{0,40}(model|table)/i, + /(extract|split|promote|create|introduce|build|spin[- ]?out|break[- ]?out)[^.]{0,60}(separate|new|dedicated)[^.]{0,40}(model|table)/i, ) || matchesUnnegated(review, /separate\s+email\s*verification\s*model/i); const acceptsInlineAddition = - /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i.test(review) || - /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i.test(review) || + matchesUnnegated(review, /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i) || + matchesUnnegated(review, /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i) || /doesn'?t (need|require|warrant)[^.]{0,40}(a\s+)?(separate|new|dedicated)[^.]{0,40}(model|table)/i.test(review); recordE2E(evalCollector, '/plan-eng-review-data-model-minimal-change', 'Plan Eng Review Data-Model Minimal Change E2E', result, { From b3acffdf582a4b703fc2f7740607f86d3150f160 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:23:13 -0700 Subject: [PATCH 10/17] fix: catch can't/won't-style contractions, guard data-model-bias against negation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainability specialist re-dispatch found the standalone n't alternative in NEGATION was unreachable: \b n't \b can never match mid-word (no word boundary exists between the letters immediately before "n" and "n" itself in "can't", "won't", "aren't", "hasn't"), so only the explicitly-spelled-out contractions (don't, doesn't, didn't, shouldn't, wouldn't, isn't) were ever detected — very common ones like "can't"/"won't"/"cannot" fell through silently. Replaced with a generic [a-z]+n't pattern plus an explicit "cannot" alternative (which has no apostrophe to match generically). Also brought the original data-model-bias test's recommendsSeparateModel check in line with its three newer sibling counterexample tests by routing it through matchesUnnegated — it was the one E2E block still un-guarded against a negated false-positive ("I would NOT recommend a separate tier model" would previously have registered as a pass). Did not touch rejectsAsUnjustified (measured-denorm test) — it directly models rejection language ("instead of denormalizing" IS the signal, not a positive claim needing negation-checking), so wrapping it in matchesUnnegated would conflict with its own tuned alternatives for no clear benefit. Did not extract a shared prompt-string constant across the 4 E2E blocks (maintainability finding) — the identical prompt string already appears verbatim in 3 pre-existing, untouched blocks in this same file; extracting a constant for only the 4 new blocks would create inconsistency rather than resolve it, and touching the pre-existing blocks is outside this PR's scope. Added unit tests reproducing both contraction gaps directly. bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — 783 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 16 ++++++++++++++++ test/helpers/e2e-helpers.ts | 7 ++++++- test/skill-e2e-plan.test.ts | 14 +++++++++----- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index 47701a6e9..0dc1b8ce0 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -62,6 +62,22 @@ describe('matchesUnnegated', () => { )).toBe(false); }); + test('recognizes "-n\'t" contractions generically (can\'t, won\'t, aren\'t, hasn\'t)', () => { + // A bare `\b n't \b` alternative can never match mid-word (no word + // boundary between the letters immediately before "n" and "n" itself), + // so only the contractions spelled out explicitly ever matched. Verify + // the generic [a-z]+n't pattern actually catches the common ones that + // previously fell through silently. + expect(matchesUnnegated("This can't promote the payload into columns.", splitPattern)).toBe(false); + expect(matchesUnnegated("This won't promote the payload into columns.", splitPattern)).toBe(false); + expect(matchesUnnegated("These aren't reasons to promote the payload into columns.", splitPattern)).toBe(false); + expect(matchesUnnegated("It hasn't been decided to promote the payload into columns.", splitPattern)).toBe(false); + }); + + test('recognizes "cannot" as a negation', () => { + expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index e09e6b352..b0187c2c0 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -156,7 +156,12 @@ export function matchesUnnegated(text: string, pattern: RegExp): boolean { // to the model RATHER THAN create a separate table") — the rejected // alternative can otherwise match the pattern just as strongly as a real // recommendation would, with no single negation word anywhere near it. - const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against|rather than|instead of)\b/i; + // [a-z]+n't matches any "-n't" contraction generically (can't, won't, + // aren't, hasn't, doesn't, don't, wouldn't, ...) — a bare `\b n't \b` + // alternative can never match mid-word (there's no word boundary between + // the letters immediately before "n" and "n" itself), so it silently + // covered nothing beyond the contractions already spelled out explicitly. + const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n't\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); let m: RegExpExecArray | null; diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 1a4dbb7f7..4004b6197 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -456,12 +456,16 @@ Focus specifically on the data model design in the plan. Apply the data model re // assumptions below. const lower = review.toLowerCase(); const unformatted = review.replace(/[`*_]/g, ''); + // matchesUnnegated so a (would-be-regression) "I would NOT recommend a + // separate tier model here" answer doesn't get counted as a pass just + // for containing the words — consistent with the sibling counterexample + // tests below. const recommendsSeparateModel = - /separate\s+(subscription\s*)?tier\s*model/i.test(unformatted) || - /new\s+(subscription\s*)?tier\s*model/i.test(unformatted) || - /subscriptiontier\s*model/i.test(unformatted) || - /extract[\s\S]*tier[\s\S]*model/i.test(unformatted) || - /tier\s*(?:table|model)\s*(?:with|per)/i.test(unformatted); + matchesUnnegated(unformatted, /separate\s+(subscription\s*)?tier\s*model/i) || + matchesUnnegated(unformatted, /new\s+(subscription\s*)?tier\s*model/i) || + matchesUnnegated(unformatted, /subscriptiontier\s*model/i) || + matchesUnnegated(unformatted, /extract[\s\S]*tier[\s\S]*model/i) || + matchesUnnegated(unformatted, /tier\s*(?:table|model)\s*(?:with|per)/i); const pushesBackOnJsonField = lower.includes('jsonfield') && (lower.includes('feature') || lower.includes('polymorph') || lower.includes('explicit')); From 02e312c0575f69053c9bc62aae03513c561fbeed Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:38:49 -0700 Subject: [PATCH 11/17] fix: harden data-model E2E fixtures against reading the operator's real gstack install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review (Step 11) flagged that these 4 tests were not hermetic: the prompt only said "Read plan-eng-review/SKILL.md" (relative, unpinned), and the copied SKILL.md's own STOP-Read directive references an ABSOLUTE path (~/.claude/skills/gstack/plan-eng-review/sections/review-sections.md) for its carved sections file. On any machine with gstack actually installed at that location — which includes the maintainer's own dev machine — the agent could read and get graded against the REAL globally-installed skill instead of this branch's sandboxed copy, silently defeating the point of the isolated fixture. This is the exact failure mode test/helpers/auq-sdk-capture.ts's setupSkillDir()/captureSectionReads() already guards against for other carved-skill tests; these 4 blocks predated that pattern and didn't use it. Fixed with a targeted change rather than a full migration to the auq-sdk-capture harness: extracted planEngReviewDataModelPrompt() into e2e-helpers.ts, pinning the sandboxed SKILL.md's absolute path and explicitly forbidding the agent from reading any other SKILL.md ("especially nothing under ~/.claude or /Users") — matching auq-sdk-capture.ts's own phrasing. This also resolves the maintainability specialist's earlier DRY finding (the identical prompt string was duplicated across all 4 blocks) in the same change, since all 4 now call the one shared function. Also fixed a second Codex finding: touchfiles.ts only listed plan-eng-review/** as these tests' dependency, but their actual behavior lives partly in test/helpers/e2e-helpers.ts (setupPlanEngReviewFixture, matchesUnnegated) — a future bug there wouldn't have gotten these 4 tests re-selected by the diff-based selector. Added the file to each test's touchfile entry (scoped to just these 4, not GLOBAL_TOUCHFILES, since the new exports aren't used elsewhere yet). Two other Codex findings reviewed and accepted as-is, not fixed: - matchesUnnegated can still mis-score idiom-based false negation ("no doubt", "not only... but also") — a genuine lexical-heuristic limitation, but these idioms are exceedingly unlikely to co-occur with the specific verb+object patterns these checks require in real review prose; chasing every conceivable idiom is diminishing returns for a best-effort matcher. - The new evals are periodic tier, not gate — consistent with this repo's existing convention for all Opus-model E2E tests (documented in CLAUDE.md's two-tier system), not a gap introduced by this branch. bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — 786 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 29 ++++++++++++++++++++ test/helpers/e2e-helpers.ts | 45 ++++++++++++++++++++++++++----- test/helpers/touchfiles.ts | 11 +++++--- test/skill-e2e-plan.test.ts | 46 ++++++++++++-------------------- 4 files changed, 92 insertions(+), 39 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index 0dc1b8ce0..a5d252daf 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -78,6 +78,35 @@ describe('matchesUnnegated', () => { expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false); }); + test('recognizes a typographic curly apostrophe ("doesn’t") as a negation', () => { + // LLM prose commonly renders contractions with U+2019 rather than the + // ASCII apostrophe — a plain [a-z]+n't pattern misses this entirely. + expect(matchesUnnegated( + 'This doesn’t need extraction — promote the payload to explicit columns is unnecessary here.', + /promote the payload to explicit columns/i, + )).toBe(false); + }); + + test('does not let an abbreviation period (e.g., i.e., etc.) hide an earlier negation', () => { + // A bare lastIndexOf('.') treats every period as a sentence break, + // including ones inside abbreviations — which would incorrectly cut the + // negation lookback window short and hide "not" from the scan. + expect(matchesUnnegated( + 'I would not recommend, e.g., promoting this payload to explicit columns.', + /promoting this payload to explicit columns/i, + )).toBe(false); + }); + + test('still treats a real sentence boundary as a boundary (does not over-widen the window)', () => { + // Guards against overcorrecting the abbreviation fix into ignoring + // genuine sentence breaks — an earlier, unrelated negation must NOT + // suppress a later, unnegated positive match. + expect(matchesUnnegated( + 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', + /promote the payload into explicit columns/i, + )).toBe(true); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index b0187c2c0..c675d0319 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -133,6 +133,28 @@ export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: strin return planDir; } +/** + * Prompt for the data-model-bias/legitimate-json/measured-denorm/minimal-change + * E2E blocks (paired with setupPlanEngReviewFixture). Pins the ABSOLUTE path to + * the sandboxed SKILL.md and explicitly forbids reading any other SKILL.md — + * especially not the operator's globally-installed ~/.claude/skills/gstack — + * because plan-eng-review/SKILL.md's own STOP-Read directive references that + * absolute path for its carved sections/ file. Without this, a machine that + * has gstack installed globally could have the agent read (and get graded + * against) the REAL installed skill instead of this branch's copy, silently + * defeating the whole point of the sandbox. + */ +export function planEngReviewDataModelPrompt(planDir: string, focusSentence: string): string { + const skillPath = path.join(planDir, 'plan-eng-review', 'SKILL.md'); + return `Read the ONLY skill file you may read, this absolute path: ${skillPath}. Do NOT Glob, find, or search for any other SKILL.md anywhere — especially nothing under ~/.claude or /Users. Follow its review workflow for this scenario. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +${focusSentence}`; +} + /** * Case-insensitive check for `pattern` in `text` that ignores matches * immediately preceded by a negation word (not/n't/no/never/without/...). @@ -161,16 +183,27 @@ export function matchesUnnegated(text: string, pattern: RegExp): boolean { // alternative can never match mid-word (there's no word boundary between // the letters immediately before "n" and "n" itself), so it silently // covered nothing beyond the contractions already spelled out explicitly. - const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n't\b/i; + // [a-z]+n['’]t matches both the ASCII apostrophe and the typographic + // curly one (’, U+2019) — LLM prose commonly renders contractions with the + // curly form, and this codebase already hit this exact gotcha once before + // (see the apostrophe wildcard note in test/spec-template-invariants.test.ts). + const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); + // Real sentence-ending punctuation is followed by whitespace + a capital + // letter, or sits at the end of the string — this excludes periods inside + // abbreviations ("e.g.", "i.e.", "etc."), decimals, and version strings, + // which a bare lastIndexOf('.') would wrongly treat as a sentence break + // and use to hide an earlier negation word from the lookback window. + const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)/g; let m: RegExpExecArray | null; while ((m = re.exec(text))) { - const sentenceStart = Math.max( - text.lastIndexOf('.', m.index), - text.lastIndexOf('!', m.index), - text.lastIndexOf('?', m.index), - ) + 1; + let sentenceStart = 0; + let em: RegExpExecArray | null; + SENTENCE_END.lastIndex = 0; + while ((em = SENTENCE_END.exec(text)) && em.index < m.index) { + sentenceStart = em.index + 1; + } const preceding = text.slice(sentenceStart, m.index); if (!NEGATION.test(preceding)) return true; if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index ea58bf4ab..accb47962 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -89,10 +89,13 @@ export const E2E_TOUCHFILES: Record = { 'plan-ceo-review-expansion-energy': ['plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'test/fixtures/mode-posture/**', 'test/helpers/llm-judge.ts'], 'plan-eng-review': ['plan-eng-review/**'], 'plan-eng-review-artifact': ['plan-eng-review/**'], - 'plan-eng-review-data-model-bias': ['plan-eng-review/**'], - 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**'], - 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**'], - 'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**'], + // Behavior lives partly in test/helpers/e2e-helpers.ts (setupPlanEngReviewFixture, + // matchesUnnegated, planEngReviewDataModelPrompt) — without this, a regression + // in that shared helper wouldn't get these tests re-selected by diff-based CI. + 'plan-eng-review-data-model-bias': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], 'plan-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'], // Plan-mode smoke tests — gate-tier safety regression tests. Each test file diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 4004b6197..76dfe110b 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -5,7 +5,7 @@ import { describeIfSelected, testConcurrentIfSelected, copyDirSync, setupBrowseShims, logCost, recordE2E, createEvalCollector, finalizeEvalCollector, - setupPlanEngReviewFixture, matchesUnnegated, + setupPlanEngReviewFixture, matchesUnnegated, planEngReviewDataModelPrompt, } from './helpers/e2e-helpers'; import { judgePosture } from './helpers/llm-judge'; import { spawnSync } from 'child_process'; @@ -424,13 +424,10 @@ column-add, no new FK navigation in the codebase. testConcurrentIfSelected('plan-eng-review-data-model-bias', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -543,13 +540,10 @@ response verbatim for audit/replay, not to model our own domain state. testConcurrentIfSelected('plan-eng-review-data-model-legitimate-json', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -643,13 +637,10 @@ not a general schema change. testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -738,13 +729,10 @@ when the verification email link is clicked. testConcurrentIfSelected('plan-eng-review-data-model-minimal-change', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, From 1820f58f698f49469e90c60a20774813750c6b76 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:45:43 -0700 Subject: [PATCH 12/17] fix: drop bare "no" as a negation trigger, treat markdown bullets as boundaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex structured review (large-diff gate, Step 11) found 3 more real gaps: - matchesUnnegated's bare "no" trigger over-fired on common positive-framing idioms: "No blocker here: create a separate tier model" and "there is no downside to promoting the payload" both got misread as negated, even though they're endorsements of the recommendation that follows, not rejections. Removed bare "no" from the negation list — "not"/"never"/ "cannot"/the "-n't" family are far more reliable negation signals and don't share this idiom-collision problem. - SENTENCE_END only recognized period-followed-by-capital as a boundary, so a bulleted recommendation like "I would not keep this inline.\n- Separate tier model" (very common LLM output formatting) let the earlier "not" leak into the bullet below it. Added a second alternative: a newline followed by a markdown bullet or numbered-list marker now also counts as a clause boundary. Fixed the boundary-index arithmetic to use the full match length (em[0].length) instead of a bare +1, since this new alternative can match more than one character ("\n- ", "\n1. "). - The minimal-change eval's own hardcoded doesn't-pattern only matched the ASCII apostrophe (`doesn'?t`), missing the curly one — inconsistent with matchesUnnegated's own apostrophe handling a few lines away in the same file. Added 3 regression tests reproducing each directly. bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — 788 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 24 ++++++++++++++++++++++++ test/helpers/e2e-helpers.ts | 19 +++++++++++++++---- test/skill-e2e-plan.test.ts | 2 +- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index a5d252daf..723e835bb 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -107,6 +107,30 @@ describe('matchesUnnegated', () => { )).toBe(true); }); + test('does not treat a bare "no" idiom as a negation ("no blocker", "no downside")', () => { + // "No blocker here: create a separate tier model" and "there's no + // downside to promoting the payload" are positive-framing idioms in + // review prose, not negations of the recommendation that follows. + expect(matchesUnnegated( + 'No blocker here: create a separate tier model.', + /create[^.]{0,60}separate[^.]{0,20}tier[^.]{0,20}model/i, + )).toBe(true); + expect(matchesUnnegated( + 'There is no downside to promoting the payload into explicit columns.', + /promoting[^.]{0,60}payload[^.]{0,60}column/i, + )).toBe(true); + }); + + test('treats a newline + markdown bullet as a clause boundary', () => { + // "I would not keep this inline.\n- Separate tier model" is common LLM + // formatting — without treating the bullet as a boundary, the "not" from + // the prose sentence above would leak into the bullet line below it. + expect(matchesUnnegated( + 'I would not keep this inline.\n- Separate subscription tier model', + /separate[^.]{0,20}subscription[^.]{0,20}tier[^.]{0,20}model/i, + )).toBe(true); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index c675d0319..0c10bfcb1 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -187,22 +187,33 @@ export function matchesUnnegated(text: string, pattern: RegExp): boolean { // curly one (’, U+2019) — LLM prose commonly renders contractions with the // curly form, and this codebase already hit this exact gotcha once before // (see the apostrophe wildcard note in test/spec-template-invariants.test.ts). - const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; + // Deliberately does NOT include a bare "no" — "no blocker here: create a + // separate model" and "no downside to promoting the payload" are common + // POSITIVE-framing idioms in review prose, not negations of what follows; + // "not"/"never"/"cannot"/the "-n't" family are far more reliable signals. + const NEGATION = /\b(not|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); // Real sentence-ending punctuation is followed by whitespace + a capital // letter, or sits at the end of the string — this excludes periods inside // abbreviations ("e.g.", "i.e.", "etc."), decimals, and version strings, // which a bare lastIndexOf('.') would wrongly treat as a sentence break - // and use to hide an earlier negation word from the lookback window. - const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)/g; + // and use to hide an earlier negation word from the lookback window. A + // newline followed by a markdown bullet/numbered-list marker is ALSO a + // clause boundary — "I would not keep this inline.\n- Separate tier model" + // is common LLM formatting, and without this the "not" from the prose + // sentence above would otherwise leak into the bullet below it. + const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)|\n\s*(?:[-*+]|\d+[.)])\s/g; let m: RegExpExecArray | null; while ((m = re.exec(text))) { let sentenceStart = 0; let em: RegExpExecArray | null; SENTENCE_END.lastIndex = 0; while ((em = SENTENCE_END.exec(text)) && em.index < m.index) { - sentenceStart = em.index + 1; + // em[0].length, not a bare +1 — the bullet-marker alternative above is + // multiple characters wide ("\n- ", "\n1. "), so a fixed +1 would leave + // part of the marker inside the scanned "preceding" window. + sentenceStart = em.index + em[0].length; } const preceding = text.slice(sentenceStart, m.index); if (!NEGATION.test(preceding)) return true; diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 76dfe110b..1883b1109 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -762,7 +762,7 @@ when the verification email link is clicked. const acceptsInlineAddition = matchesUnnegated(review, /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i) || matchesUnnegated(review, /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i) || - /doesn'?t (need|require|warrant)[^.]{0,40}(a\s+)?(separate|new|dedicated)[^.]{0,40}(model|table)/i.test(review); + /doesn['’]?t (need|require|warrant)[^.]{0,40}(a\s+)?(separate|new|dedicated)[^.]{0,40}(model|table)/i.test(review); recordE2E(evalCollector, '/plan-eng-review-data-model-minimal-change', 'Plan Eng Review Data-Model Minimal Change E2E', result, { passed: From c58e1b073b9669d2dbbadb08fed505323acf7449 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 22:05:31 -0700 Subject: [PATCH 13/17] chore: bump version and changelog (v1.61.0.0) Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 23 +++++++++++++++++++++++ TODOS.md | 13 +++++++++++++ VERSION | 2 +- package.json | 2 +- 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 351e7cabd..132a673ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## [1.61.0.0] - 2026-07-16 + +## **`/plan-eng-review` and `/plan-ceo-review` stop pushing you toward fewer tables and JSONField shortcuts — without swinging to the opposite extreme.** + +Both skills' "right-sized diff" preference was quietly biased against schema normalization: adding a new model touches more files than adding a column, so reviewers kept steering people toward merging new fields into existing tables and reaching for JSONField as a polymorphism shortcut. This release adds an explicit data-model exception — normalize first, denormalize when you have a measured reason — plus a full "Data model review checklist" in `/plan-eng-review`'s Architecture Review (SRP, nullable-semantic smells, hidden polymorphism, FK deletion strategy, and seven more). Early feedback flagged the fix as too absolute in the other direction, so this release also adds explicit counterexamples: legitimate JSONField uses (caching a third-party webhook payload verbatim, an open-ended preference bag), denormalization backed by a real measurement (a profiled hot path, a load test), and the case where keeping a single trivial field inline is genuinely the right, minimal call. + +### What this means for you + +Ask `/plan-eng-review` to review a schema change and it will push back on merging polymorphic data into an existing model or reaching for JSONField when the variants are already known — but it will also recognize when JSONField is the correct choice (a verbatim third-party payload cache), when denormalizing is justified (you've measured the bottleneck), and when a single new field genuinely doesn't need its own table. + +### Itemized changes + +#### Changed +- `/plan-eng-review` and `/plan-ceo-review`: added a "data model exception to right-sized diff" — normalize first, denormalize for a measured reason, don't split reflexively when a field has no independent query pattern or consumer. +- `/plan-eng-review` and `/plan-ceo-review`: added explicit JSONField guidance — not an escape hatch for known, stable polymorphism, but legitimately fine for third-party payload caches, open-ended preference bags, and schemas still being discovered. +- `/plan-eng-review`: Architecture Review gained a "Data model honesty" check and a full Data Model Review Checklist (11 proactive checks) that runs on every model touched by a plan, instead of waiting for the user to push back on model shape. +- `/plan-eng-review`: three new cognitive patterns (normalize-first, SRP-for-data-models, structure-beats-blobs-for-known-polymorphism). + +#### For contributors +- New periodic-tier E2E evals exercising all three review-feedback categories end-to-end: a plan that should trigger pushback (polymorphic data inlined with JSONField), one that shouldn't (a legitimate third-party payload cache), one testing measured denormalization is accepted, and one testing a minimal single-field addition isn't over-split. +- New free unit tests for the negation-aware text matcher these evals rely on (`test/helpers/e2e-helpers.test.ts`), covering sentence-boundary scanning, curly-apostrophe contractions, contrastive phrasing ("rather than"/"instead of"), and markdown-bullet clause boundaries — all real gaps found across four rounds of specialist and adversarial review during this branch's own `/ship`. +- Extracted shared E2E fixture setup (`setupPlanEngReviewFixture`) and a hermetic, absolute-path-pinned prompt builder (`planEngReviewDataModelPrompt`) so these new tests can't accidentally grade against a machine's globally-installed skill instead of the branch under test. + ## [1.60.1.0] - 2026-07-09 ## **The /autoplan dual-voice eval is back on the board, catching real regressions.** diff --git a/TODOS.md b/TODOS.md index 29721e6e5..706ba0c23 100644 --- a/TODOS.md +++ b/TODOS.md @@ -735,6 +735,19 @@ scope of that PR; deliberately deferred to keep PTY-import small. ## Testing +## P2: /plan-ceo-review data-model-bias behavioral E2E coverage + +**What:** The v1.61.0.0 schema-consolidation-bias fix (garrytan/gstack#1048) added the same two data-model preference bullets ("right-sized diff" exception + "JSONField not an escape hatch") to both `/plan-eng-review` and `/plan-ceo-review`, but the four new periodic-tier E2E evals (`plan-eng-review-data-model-{bias,legitimate-json,measured-denorm,minimal-change}`) only drive `/plan-eng-review`. `/plan-ceo-review`'s copy of the same bullets has zero behavioral test coverage — only the static grep guardrail in `test/skill-validation.test.ts` confirms the text still exists. + +**Why:** `/plan-ceo-review` runs a materially different workflow (SCOPE EXPANSION / SELECTIVE EXPANSION / HOLD SCOPE modes, its own `sections/review-sections.md`) — identical prose is no guarantee the model actually applies the bias fix the same way it does in `/plan-eng-review`. `touchfiles.ts` doesn't map any test to a change in these bullets for plan-ceo-review, so a regression there would go unnoticed by diff-based test selection entirely. Flagged by the ship-workflow coverage audit during this branch's own `/ship` run. + +**Context:** Mirror the four `plan-eng-review-data-model-*` blocks in `test/skill-e2e-plan.test.ts` for plan-ceo-review — same synthetic plans, same `setupPlanEngReviewFixture`-style hermetic fixture (would need a `plan-ceo-review` variant), same `matchesUnnegated`-guarded regex assertions. plan-ceo-review's HOLD SCOPE mode is the simplest one that still exercises the Architecture Review, per the pattern already used in `test/skill-e2e-plan-ceo-review-section-loading.test.ts`. + +**Priority:** P2. +**Effort:** M (human: ~half day / CC: ~1-2h — mostly copy-adapt of the existing plan-eng-review blocks). + +--- + ## P2: Per-finding AskUserQuestion count assertion for /plan-ceo-review **What:** PTY E2E test that drives /plan-ceo-review through Step 0 with a stable fixture diff containing N known findings, asserts that exactly N distinct AskUserQuestions fire (one per finding) before plan_ready. diff --git a/VERSION b/VERSION index c4190e004..82b3fa4c6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.60.1.0 +1.61.0.0 diff --git a/package.json b/package.json index 846438a60..9ffb1caf7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.60.1.0", + "version": "1.61.0.0", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", From f3c54078aacb41cb089fab3d23dab1bae188d79c Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 22:16:25 -0700 Subject: [PATCH 14/17] docs: clarify new E2E evals are scoped to /plan-eng-review only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex cross-model doc review (via /document-release, run as part of /ship Step 18) flagged that the CHANGELOG's contributor bullet about the new periodic-tier E2E evals didn't scope them to /plan-eng-review — could be misread as covering /plan-ceo-review too, which still only has static text coverage (tracked as a P2 follow-up in TODOS.md). --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 132a673ba..f169b7ce3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ Ask `/plan-eng-review` to review a schema change and it will push back on mergin - `/plan-eng-review`: three new cognitive patterns (normalize-first, SRP-for-data-models, structure-beats-blobs-for-known-polymorphism). #### For contributors -- New periodic-tier E2E evals exercising all three review-feedback categories end-to-end: a plan that should trigger pushback (polymorphic data inlined with JSONField), one that shouldn't (a legitimate third-party payload cache), one testing measured denormalization is accepted, and one testing a minimal single-field addition isn't over-split. +- New periodic-tier E2E evals, all against `/plan-eng-review` (not yet `/plan-ceo-review` — tracked as a follow-up in TODOS.md), exercising all three review-feedback categories end-to-end: a plan that should trigger pushback (polymorphic data inlined with JSONField), one that shouldn't (a legitimate third-party payload cache), one testing measured denormalization is accepted, and one testing a minimal single-field addition isn't over-split. - New free unit tests for the negation-aware text matcher these evals rely on (`test/helpers/e2e-helpers.test.ts`), covering sentence-boundary scanning, curly-apostrophe contractions, contrastive phrasing ("rather than"/"instead of"), and markdown-bullet clause boundaries — all real gaps found across four rounds of specialist and adversarial review during this branch's own `/ship`. - Extracted shared E2E fixture setup (`setupPlanEngReviewFixture`) and a hermetic, absolute-path-pinned prompt builder (`planEngReviewDataModelPrompt`) so these new tests can't accidentally grade against a machine's globally-installed skill instead of the branch under test. From 54a50e4b67273d1920050050c90a0f13e06849a7 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 23:25:08 -0700 Subject: [PATCH 15/17] =?UTF-8?q?fix(plan-eng-review,plan-ceo-review):=20t?= =?UTF-8?q?rim=20scope=20per=20PR=20review=20=E2=80=94=20drop=20CEO=20bull?= =?UTF-8?q?ets,=20own=20section,=20fewer=20patterns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- plan-ceo-review/SKILL.md | 2 - plan-ceo-review/SKILL.md.tmpl | 2 - plan-eng-review/SKILL.md | 17 ++++---- plan-eng-review/SKILL.md.tmpl | 17 ++++---- plan-eng-review/sections/review-sections.md | 9 ++++- .../sections/review-sections.md.tmpl | 9 ++++- test/helpers/carve-guards.ts | 29 +++++++------- test/skill-validation.test.ts | 39 +++++-------------- 8 files changed, 51 insertions(+), 73 deletions(-) diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index afed8f46f..3d3208bee 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -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. diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index fe7617d81..c43cfe641 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -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. diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index 45d1f2516..07cb0ebf4 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -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. diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index f66a35baa..2f127916c 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -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. diff --git a/plan-eng-review/sections/review-sections.md b/plan-eng-review/sections/review-sections.md index 7ccc56266..9b0b30b67 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -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." diff --git a/plan-eng-review/sections/review-sections.md.tmpl b/plan-eng-review/sections/review-sections.md.tmpl index 65bb9b513..8e05878ac 100644 --- a/plan-eng-review/sections/review-sections.md.tmpl +++ b/plan-eng-review/sections/review-sections.md.tmpl @@ -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." diff --git a/test/helpers/carve-guards.ts b/test/helpers/carve-guards.ts index 34c4f82ad..372f3c523 100644 --- a/test/helpers/carve-guards.ts +++ b/test/helpers/carve-guards.ts @@ -144,11 +144,11 @@ export const CARVE_GUARDS: Record = { }, 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 = { }, 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', diff --git a/test/skill-validation.test.ts b/test/skill-validation.test.ts index 3e0c0246c..d11d29aac 100644 --- a/test/skill-validation.test.ts +++ b/test/skill-validation.test.ts @@ -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); }); }); From b39da7712956a6ab875724d7b00b99129e122e8d Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 23:25:22 -0700 Subject: [PATCH 16/17] test: simplify matchesUnnegated, drop its dedicated unit test file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per PR review feedback: the negation-aware matcher these E2E evals rely on had accumulated real complexity (sentence-boundary scanning, markdown-bullet clause detection, curly-apostrophe contraction matching, idiom exclusions) across four rounds of specialist/adversarial review during this branch's own /ship — each finding was real, but chasing all of them was disproportionate for a periodic-tier, non-gating eval. An occasional missed edge case there just means a rare, visible false read, not a missed real regression. Reverted to a plain fixed-window lookback (60 chars, accounting for the patterns' own internal gaps) with a short negation word list. Kept the generic [a-z]+n't contraction match (can't/won't/aren't) since it's one regex alternative, not meaningful added complexity. Removed test/helpers/e2e-helpers.test.ts — a dedicated unit-test file for a best-effort text-matching heuristic used only by 4 paid, periodic-tier evals was more test infrastructure than the actual ask (3-4 new eval cases) warranted. --- test/helpers/e2e-helpers.test.ts | 204 ------------------------------- test/helpers/e2e-helpers.ts | 58 ++------- 2 files changed, 8 insertions(+), 254 deletions(-) delete mode 100644 test/helpers/e2e-helpers.test.ts diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts deleted file mode 100644 index 723e835bb..000000000 --- a/test/helpers/e2e-helpers.test.ts +++ /dev/null @@ -1,204 +0,0 @@ -/** - * Free, deterministic unit tests for the pure/local helpers added alongside - * the schema-consolidation-bias E2E evals. matchesUnnegated() and - * setupPlanEngReviewFixture() were previously only exercised indirectly by - * the paid, EVALS=1-gated E2E tests in skill-e2e-plan.test.ts — a bug in - * either could silently flip an eval's pass/fail verdict and be - * indistinguishable from ordinary LLM wording variance. These tests run in - * the free `bun test` suite instead. - */ - -import { describe, test, expect } from 'bun:test'; -import * as fs from 'fs'; -import { matchesUnnegated, setupPlanEngReviewFixture, ROOT } from './e2e-helpers'; -import * as path from 'path'; - -describe('matchesUnnegated', () => { - const splitPattern = /(promote|split|convert|extract|move|normali[sz]e)[^.]{0,80}payload[^.]{0,80}(column|field)/i; - - test('finds an unnegated positive match', () => { - expect(matchesUnnegated( - 'I recommend you promote the payload into explicit columns for the common fields.', - splitPattern, - )).toBe(true); - }); - - test('ignores a match preceded by "not"', () => { - expect(matchesUnnegated( - 'I would not extract the payload into columns, since the schema is Stripe-controlled.', - splitPattern, - )).toBe(false); - }); - - test('ignores a match preceded by "n\'t" (doesn\'t / wouldn\'t / shouldn\'t)', () => { - expect(matchesUnnegated( - 'This wouldn\'t promote the payload into columns.', - splitPattern, - )).toBe(false); - }); - - test('ignores a match preceded by "without"', () => { - expect(matchesUnnegated( - 'The payload stays as-is without splitting the payload into columns.', - splitPattern, - )).toBe(false); - }); - - test('ignores a match that is the rejected half of a "rather than" contrast', () => { - // "Keep it on the existing model RATHER THAN promote the payload into - // columns" — the rejected alternative matches the pattern just as - // strongly as a real recommendation would, with no single negation - // word (not/never/without/...) anywhere near it. - expect(matchesUnnegated( - 'Keep this field on the existing model rather than promote the payload into explicit columns.', - splitPattern, - )).toBe(false); - }); - - test('ignores a match that is the rejected half of an "instead of" contrast', () => { - expect(matchesUnnegated( - 'Keep this on the existing model instead of splitting the payload into columns.', - splitPattern, - )).toBe(false); - }); - - test('recognizes "-n\'t" contractions generically (can\'t, won\'t, aren\'t, hasn\'t)', () => { - // A bare `\b n't \b` alternative can never match mid-word (no word - // boundary between the letters immediately before "n" and "n" itself), - // so only the contractions spelled out explicitly ever matched. Verify - // the generic [a-z]+n't pattern actually catches the common ones that - // previously fell through silently. - expect(matchesUnnegated("This can't promote the payload into columns.", splitPattern)).toBe(false); - expect(matchesUnnegated("This won't promote the payload into columns.", splitPattern)).toBe(false); - expect(matchesUnnegated("These aren't reasons to promote the payload into columns.", splitPattern)).toBe(false); - expect(matchesUnnegated("It hasn't been decided to promote the payload into columns.", splitPattern)).toBe(false); - }); - - test('recognizes "cannot" as a negation', () => { - expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false); - }); - - test('recognizes a typographic curly apostrophe ("doesn’t") as a negation', () => { - // LLM prose commonly renders contractions with U+2019 rather than the - // ASCII apostrophe — a plain [a-z]+n't pattern misses this entirely. - expect(matchesUnnegated( - 'This doesn’t need extraction — promote the payload to explicit columns is unnecessary here.', - /promote the payload to explicit columns/i, - )).toBe(false); - }); - - test('does not let an abbreviation period (e.g., i.e., etc.) hide an earlier negation', () => { - // A bare lastIndexOf('.') treats every period as a sentence break, - // including ones inside abbreviations — which would incorrectly cut the - // negation lookback window short and hide "not" from the scan. - expect(matchesUnnegated( - 'I would not recommend, e.g., promoting this payload to explicit columns.', - /promoting this payload to explicit columns/i, - )).toBe(false); - }); - - test('still treats a real sentence boundary as a boundary (does not over-widen the window)', () => { - // Guards against overcorrecting the abbreviation fix into ignoring - // genuine sentence breaks — an earlier, unrelated negation must NOT - // suppress a later, unnegated positive match. - expect(matchesUnnegated( - 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', - /promote the payload into explicit columns/i, - )).toBe(true); - }); - - test('does not treat a bare "no" idiom as a negation ("no blocker", "no downside")', () => { - // "No blocker here: create a separate tier model" and "there's no - // downside to promoting the payload" are positive-framing idioms in - // review prose, not negations of the recommendation that follows. - expect(matchesUnnegated( - 'No blocker here: create a separate tier model.', - /create[^.]{0,60}separate[^.]{0,20}tier[^.]{0,20}model/i, - )).toBe(true); - expect(matchesUnnegated( - 'There is no downside to promoting the payload into explicit columns.', - /promoting[^.]{0,60}payload[^.]{0,60}column/i, - )).toBe(true); - }); - - test('treats a newline + markdown bullet as a clause boundary', () => { - // "I would not keep this inline.\n- Separate tier model" is common LLM - // formatting — without treating the bullet as a boundary, the "not" from - // the prose sentence above would leak into the bullet line below it. - expect(matchesUnnegated( - 'I would not keep this inline.\n- Separate subscription tier model', - /separate[^.]{0,20}subscription[^.]{0,20}tier[^.]{0,20}model/i, - )).toBe(true); - }); - - test('catches a positive match even when an unrelated negation appears earlier in the text', () => { - expect(matchesUnnegated( - 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', - splitPattern, - )).toBe(true); - }); - - test('finds a positive match after a negated one in an earlier sentence (does not short-circuit on the first match)', () => { - // First sentence is negated ("not extract..."), second is a genuine - // recommendation ("should promote..."). The period between them bounds - // the pattern's [^.]{0,80} window to one sentence each, so this must - // find two distinct matches — matchesUnnegated must not stop scanning - // after the first (negated) hit. - expect(matchesUnnegated( - 'I would not extract the payload into columns for the rare fields. For the common fields, I would promote the payload into explicit columns.', - splitPattern, - )).toBe(true); - }); - - test('returns false when the pattern never matches at all', () => { - expect(matchesUnnegated('This review has nothing to do with payloads or columns.', splitPattern)).toBe(false); - }); - - test('ignores a negation far earlier in the same (period-free) sentence', () => { - // The negation-lookback scans to the sentence boundary, not a fixed - // character count — this is the scenario that motivated that design. - // "not" sits well over 20 characters before "extract...payload...columns" - // but is still part of the same sentence (no period between them). - expect(matchesUnnegated( - "I do not think it's worth the added complexity of maintaining a separate table just to extract the payload into columns for these rare cases.", - splitPattern, - )).toBe(false); - }); - - test('does not infinite-loop on a zero-width-adjacent pattern', () => { - // Guards the re.lastIndex++ zero-width-match protection. Note: a bare - // quantifier pattern like /x*/ always produces an empty match at index 0 - // with nothing preceding it (so it's trivially "unnegated" and returns - // true immediately) — this test's value is that it terminates at all, - // not that it exercises the negated branch of the loop. - const zeroWidthish = /x*/i; - expect(() => matchesUnnegated('no x here', zeroWidthish)).not.toThrow(); - }); - - test('accepts a pattern that already has the global flag', () => { - const globalPattern = /payload/gi; - expect(matchesUnnegated('the payload arrived', globalPattern)).toBe(true); - }); -}); - -describe('setupPlanEngReviewFixture', () => { - test('creates a git repo with plan.md, SKILL.md, and sections/ copied from the real skill', () => { - const planDir = setupPlanEngReviewFixture('e2e-helpers-test-fixture-', '# Plan: test fixture\n\nSome content.\n'); - try { - expect(fs.existsSync(path.join(planDir, '.git'))).toBe(true); - expect(fs.readFileSync(path.join(planDir, 'plan.md'), 'utf-8')).toContain('Plan: test fixture'); - expect(fs.existsSync(path.join(planDir, 'plan-eng-review', 'SKILL.md'))).toBe(true); - - const realSkillMd = fs.readFileSync(path.join(ROOT, 'plan-eng-review', 'SKILL.md'), 'utf-8'); - const copiedSkillMd = fs.readFileSync(path.join(planDir, 'plan-eng-review', 'SKILL.md'), 'utf-8'); - expect(copiedSkillMd).toBe(realSkillMd); - - const realSectionsDir = path.join(ROOT, 'plan-eng-review', 'sections'); - if (fs.existsSync(realSectionsDir)) { - expect(fs.existsSync(path.join(planDir, 'plan-eng-review', 'sections', 'review-sections.md'))).toBe(true); - } - } finally { - fs.rmSync(planDir, { recursive: true, force: true }); - } - }); -}); diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index 0c10bfcb1..8bb54bab1 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -157,65 +157,23 @@ ${focusSentence}`; /** * Case-insensitive check for `pattern` in `text` that ignores matches - * immediately preceded by a negation word (not/n't/no/never/without/...). + * immediately preceded by a negation word (not/n't/never/without/...). * Plain substring/regex matching can't tell "extract the payload into * columns" from "would NOT extract the payload into columns" — this walks - * every match and inspects the text just before it for a negation cue. + * every match and checks the text just before it for a negation cue. * - * The negation lookback scans back to the start of the CURRENT SENTENCE - * (the last `.`/`!`/`?` before the match) rather than a fixed character - * count. A fixed window (originally 20 chars) is too narrow: the patterns - * this is used with have their own internal gaps of up to 80 chars (e.g. - * `(verb)[^.]{0,80}payload[^.]{0,80}column`), so a negation word like "not" - * can legitimately sit further back in the same sentence than a small fixed - * window would see — "I do not think it's worth extracting the payload into - * columns" would otherwise be misread as an unnegated recommendation. - * Scanning to the sentence boundary matches the same period-bounded - * assumption the calling patterns already make. + * Deliberately simple: a fixed lookback window, not full sentence-boundary + * parsing. These evals are periodic-tier (paid, non-gating) — an occasional + * missed edge case just means a rare, visible false read on a test that + * doesn't block anything, which is a fine trade for keeping this readable. */ export function matchesUnnegated(text: string, pattern: RegExp): boolean { - // "rather than"/"instead of" catch contrastive phrasing ("add this field - // to the model RATHER THAN create a separate table") — the rejected - // alternative can otherwise match the pattern just as strongly as a real - // recommendation would, with no single negation word anywhere near it. - // [a-z]+n't matches any "-n't" contraction generically (can't, won't, - // aren't, hasn't, doesn't, don't, wouldn't, ...) — a bare `\b n't \b` - // alternative can never match mid-word (there's no word boundary between - // the letters immediately before "n" and "n" itself), so it silently - // covered nothing beyond the contractions already spelled out explicitly. - // [a-z]+n['’]t matches both the ASCII apostrophe and the typographic - // curly one (’, U+2019) — LLM prose commonly renders contractions with the - // curly form, and this codebase already hit this exact gotcha once before - // (see the apostrophe wildcard note in test/spec-template-invariants.test.ts). - // Deliberately does NOT include a bare "no" — "no blocker here: create a - // separate model" and "no downside to promoting the payload" are common - // POSITIVE-framing idioms in review prose, not negations of what follows; - // "not"/"never"/"cannot"/the "-n't" family are far more reliable signals. - const NEGATION = /\b(not|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; + const NEGATION = /\b(not|never|cannot|without|rather than|instead of)\b|[a-z]+n't\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); - // Real sentence-ending punctuation is followed by whitespace + a capital - // letter, or sits at the end of the string — this excludes periods inside - // abbreviations ("e.g.", "i.e.", "etc."), decimals, and version strings, - // which a bare lastIndexOf('.') would wrongly treat as a sentence break - // and use to hide an earlier negation word from the lookback window. A - // newline followed by a markdown bullet/numbered-list marker is ALSO a - // clause boundary — "I would not keep this inline.\n- Separate tier model" - // is common LLM formatting, and without this the "not" from the prose - // sentence above would otherwise leak into the bullet below it. - const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)|\n\s*(?:[-*+]|\d+[.)])\s/g; let m: RegExpExecArray | null; while ((m = re.exec(text))) { - let sentenceStart = 0; - let em: RegExpExecArray | null; - SENTENCE_END.lastIndex = 0; - while ((em = SENTENCE_END.exec(text)) && em.index < m.index) { - // em[0].length, not a bare +1 — the bullet-marker alternative above is - // multiple characters wide ("\n- ", "\n1. "), so a fixed +1 would leave - // part of the marker inside the scanned "preceding" window. - sentenceStart = em.index + em[0].length; - } - const preceding = text.slice(sentenceStart, m.index); + const preceding = text.slice(Math.max(0, m.index - 60), m.index); if (!NEGATION.test(preceding)) return true; if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches } From 72cae55a72d790e504639a8db76899b608c1db13 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 23:25:29 -0700 Subject: [PATCH 17/17] docs: update CHANGELOG and TODOS to match the trimmed scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG no longer claims plan-ceo-review changes (bullets were dropped entirely) and reflects the Data Model Review's promotion to its own section instead of 11 checklist items + 3 cognitive patterns. Removed the P2 TODO for /plan-ceo-review data-model-bias E2E coverage — moot now that plan-ceo-review carries none of these bullets to test. --- CHANGELOG.md | 16 +++++++--------- TODOS.md | 13 ------------- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f169b7ce3..2bca912cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,9 @@ ## [1.61.0.0] - 2026-07-16 -## **`/plan-eng-review` and `/plan-ceo-review` stop pushing you toward fewer tables and JSONField shortcuts — without swinging to the opposite extreme.** +## **`/plan-eng-review` stops pushing you toward fewer tables and JSONField shortcuts — without swinging to the opposite extreme.** -Both skills' "right-sized diff" preference was quietly biased against schema normalization: adding a new model touches more files than adding a column, so reviewers kept steering people toward merging new fields into existing tables and reaching for JSONField as a polymorphism shortcut. This release adds an explicit data-model exception — normalize first, denormalize when you have a measured reason — plus a full "Data model review checklist" in `/plan-eng-review`'s Architecture Review (SRP, nullable-semantic smells, hidden polymorphism, FK deletion strategy, and seven more). Early feedback flagged the fix as too absolute in the other direction, so this release also adds explicit counterexamples: legitimate JSONField uses (caching a third-party webhook payload verbatim, an open-ended preference bag), denormalization backed by a real measurement (a profiled hot path, a load test), and the case where keeping a single trivial field inline is genuinely the right, minimal call. +The "right-sized diff" preference was quietly biased against schema normalization: adding a new model touches more files than adding a column, so the skill kept steering people toward merging new fields into existing tables and reaching for JSONField as a polymorphism shortcut. This release adds a terse data-model exception to that preference — normalize first, denormalize when you have a measured reason — plus a new, standalone "Data model review" section (11 proactive checks: SRP, nullable-semantic smells, hidden polymorphism, FK deletion strategy, and seven more) that runs on every plan touching a model. Early feedback flagged the fix as too absolute in the other direction, so this release also adds explicit counterexamples: legitimate JSONField uses (caching a third-party webhook payload verbatim, an open-ended preference bag), denormalization backed by a real measurement (a profiled hot path, a load test), and the case where keeping a single trivial field inline is genuinely the right, minimal call. ### What this means for you @@ -13,15 +13,13 @@ Ask `/plan-eng-review` to review a schema change and it will push back on mergin ### Itemized changes #### Changed -- `/plan-eng-review` and `/plan-ceo-review`: added a "data model exception to right-sized diff" — normalize first, denormalize for a measured reason, don't split reflexively when a field has no independent query pattern or consumer. -- `/plan-eng-review` and `/plan-ceo-review`: added explicit JSONField guidance — not an escape hatch for known, stable polymorphism, but legitimately fine for third-party payload caches, open-ended preference bags, and schemas still being discovered. -- `/plan-eng-review`: Architecture Review gained a "Data model honesty" check and a full Data Model Review Checklist (11 proactive checks) that runs on every model touched by a plan, instead of waiting for the user to push back on model shape. -- `/plan-eng-review`: three new cognitive patterns (normalize-first, SRP-for-data-models, structure-beats-blobs-for-known-polymorphism). +- `/plan-eng-review`: added a terse "data model exception to right-sized diff" preference — normalize first, denormalize for a measured reason, don't split reflexively when a field has no independent query pattern or consumer. +- `/plan-eng-review`: added a terse JSONField preference — not an escape hatch for known, stable polymorphism, but legitimately fine for third-party payload caches, open-ended preference bags, and schemas still being discovered. +- `/plan-eng-review`: the Data Model Review Checklist is now its own standalone review section (previously a subsection tacked onto Architecture Review), covering SRP, nullable-semantic smells, hidden polymorphism, FK deletion strategy, snapshot-vs-render-live, cross-scope FK consistency, derived state, field naming, and DB constraints over app validation. #### For contributors -- New periodic-tier E2E evals, all against `/plan-eng-review` (not yet `/plan-ceo-review` — tracked as a follow-up in TODOS.md), exercising all three review-feedback categories end-to-end: a plan that should trigger pushback (polymorphic data inlined with JSONField), one that shouldn't (a legitimate third-party payload cache), one testing measured denormalization is accepted, and one testing a minimal single-field addition isn't over-split. -- New free unit tests for the negation-aware text matcher these evals rely on (`test/helpers/e2e-helpers.test.ts`), covering sentence-boundary scanning, curly-apostrophe contractions, contrastive phrasing ("rather than"/"instead of"), and markdown-bullet clause boundaries — all real gaps found across four rounds of specialist and adversarial review during this branch's own `/ship`. -- Extracted shared E2E fixture setup (`setupPlanEngReviewFixture`) and a hermetic, absolute-path-pinned prompt builder (`planEngReviewDataModelPrompt`) so these new tests can't accidentally grade against a machine's globally-installed skill instead of the branch under test. +- New periodic-tier E2E evals against `/plan-eng-review`, exercising all three review-feedback categories end-to-end: a plan that should trigger pushback (polymorphic data inlined with JSONField), one that shouldn't (a legitimate third-party payload cache), one testing measured denormalization is accepted, and one testing a minimal single-field addition isn't over-split. +- `/plan-ceo-review` deliberately does not carry these bullets — CEO review stays high-level on "right-sized diff"; data-model pushback is `/plan-eng-review`'s job. ## [1.60.1.0] - 2026-07-09 diff --git a/TODOS.md b/TODOS.md index 706ba0c23..29721e6e5 100644 --- a/TODOS.md +++ b/TODOS.md @@ -735,19 +735,6 @@ scope of that PR; deliberately deferred to keep PTY-import small. ## Testing -## P2: /plan-ceo-review data-model-bias behavioral E2E coverage - -**What:** The v1.61.0.0 schema-consolidation-bias fix (garrytan/gstack#1048) added the same two data-model preference bullets ("right-sized diff" exception + "JSONField not an escape hatch") to both `/plan-eng-review` and `/plan-ceo-review`, but the four new periodic-tier E2E evals (`plan-eng-review-data-model-{bias,legitimate-json,measured-denorm,minimal-change}`) only drive `/plan-eng-review`. `/plan-ceo-review`'s copy of the same bullets has zero behavioral test coverage — only the static grep guardrail in `test/skill-validation.test.ts` confirms the text still exists. - -**Why:** `/plan-ceo-review` runs a materially different workflow (SCOPE EXPANSION / SELECTIVE EXPANSION / HOLD SCOPE modes, its own `sections/review-sections.md`) — identical prose is no guarantee the model actually applies the bias fix the same way it does in `/plan-eng-review`. `touchfiles.ts` doesn't map any test to a change in these bullets for plan-ceo-review, so a regression there would go unnoticed by diff-based test selection entirely. Flagged by the ship-workflow coverage audit during this branch's own `/ship` run. - -**Context:** Mirror the four `plan-eng-review-data-model-*` blocks in `test/skill-e2e-plan.test.ts` for plan-ceo-review — same synthetic plans, same `setupPlanEngReviewFixture`-style hermetic fixture (would need a `plan-ceo-review` variant), same `matchesUnnegated`-guarded regex assertions. plan-ceo-review's HOLD SCOPE mode is the simplest one that still exercises the Architecture Review, per the pattern already used in `test/skill-e2e-plan-ceo-review-section-loading.test.ts`. - -**Priority:** P2. -**Effort:** M (human: ~half day / CC: ~1-2h — mostly copy-adapt of the existing plan-eng-review blocks). - ---- - ## P2: Per-finding AskUserQuestion count assertion for /plan-ceo-review **What:** PTY E2E test that drives /plan-ceo-review through Step 0 with a stable fixture diff containing N known findings, asserts that exactly N distinct AskUserQuestions fire (one per finding) before plan_ready.