mirror of https://github.com/garrytan/gstack.git
Merge 72cae55a72 into a3259400a3
This commit is contained in:
David Grant
GitHub
commit
66640a4efa
21
CHANGELOG.md
21
CHANGELOG.md
|
|
@ -1,5 +1,26 @@
|
|||
# Changelog
|
||||
|
||||
## [1.61.0.0] - 2026-07-16
|
||||
|
||||
## **`/plan-eng-review` stops pushing you toward fewer tables and JSONField shortcuts — without swinging to the opposite extreme.**
|
||||
|
||||
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
|
||||
|
||||
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`: 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 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
|
||||
|
||||
## **The /autoplan dual-voice eval is back on the board, catching real regressions.**
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -834,6 +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":** 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
|
||||
|
||||
|
|
|
|||
|
|
@ -62,6 +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":** 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
|
||||
|
||||
|
|
|
|||
|
|
@ -59,6 +59,72 @@ For each issue found in this section, call AskUserQuestion individually. One iss
|
|||
|
||||
**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."
|
||||
|
||||
**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 — 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?
|
||||
- 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," 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?
|
||||
- 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.
|
||||
|
||||
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.
|
||||
|
||||
## Confidence Calibration
|
||||
|
||||
Every finding MUST include a confidence score (1-10):
|
||||
|
|
|
|||
|
|
@ -21,6 +21,72 @@ For each issue found in this section, call AskUserQuestion individually. One iss
|
|||
|
||||
**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."
|
||||
|
||||
**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 — 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?
|
||||
- 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," 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?
|
||||
- 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.
|
||||
|
||||
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.
|
||||
|
||||
{{CONFIDENCE_CALIBRATION}}
|
||||
|
||||
### 2. Code quality review
|
||||
|
|
|
|||
|
|
@ -144,6 +144,10 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
|||
},
|
||||
behavioral: 'external',
|
||||
externalTest: 'test/skill-e2e-plan-ceo-review-section-loading.test.ts',
|
||||
// Data-model bias fix (garrytan/gstack#1048): 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'],
|
||||
|
|
@ -164,15 +168,20 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
|||
},
|
||||
behavioral: 'plan',
|
||||
// v1.2.0 activation lift (shared first-run-guidance preamble) + #2077 ask-first scope gate.
|
||||
maxSkeletonBytes: 67_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.
|
||||
maxSizeRatio: 1.08,
|
||||
// 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',
|
||||
|
|
|
|||
|
|
@ -102,6 +102,84 @@ 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;
|
||||
}
|
||||
|
||||
/**
|
||||
* 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/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 checks the text just before it for a negation cue.
|
||||
*
|
||||
* 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 {
|
||||
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);
|
||||
let m: RegExpExecArray | null;
|
||||
while ((m = re.exec(text))) {
|
||||
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
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Set up browse shims (binary symlink, find-browse, remote-slug) in a tmpDir.
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -89,6 +89,13 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
|||
'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/**'],
|
||||
// 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
|
||||
|
|
@ -520,6 +527,10 @@ export const E2E_TIERS: Record<string, 'gate' | 'periodic'> = {
|
|||
'plan-ceo-review-expansion-energy': 'gate', // V1.1 mode-posture regression gate (Opus generator, Sonnet judge)
|
||||
'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-review-data-model-minimal-change': 'periodic',
|
||||
'plan-eng-coverage-audit': 'gate',
|
||||
'plan-review-report': 'gate',
|
||||
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import {
|
|||
describeIfSelected, testConcurrentIfSelected,
|
||||
copyDirSync, setupBrowseShims, logCost, recordE2E,
|
||||
createEvalCollector, finalizeEvalCollector,
|
||||
setupPlanEngReviewFixture, matchesUnnegated, planEngReviewDataModelPrompt,
|
||||
} from './helpers/e2e-helpers';
|
||||
import { judgePosture } from './helpers/llm-judge';
|
||||
import { spawnSync } from 'child_process';
|
||||
|
|
@ -366,6 +367,420 @@ 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 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;
|
||||
|
||||
beforeAll(() => {
|
||||
// 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.
|
||||
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
|
||||
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?
|
||||
`);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {}
|
||||
});
|
||||
|
||||
testConcurrentIfSelected('plan-eng-review-data-model-bias', async () => {
|
||||
const result = await runSkillTest({
|
||||
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,
|
||||
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. 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, '');
|
||||
// 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 =
|
||||
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'));
|
||||
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 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(() => {
|
||||
// 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.
|
||||
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
|
||||
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?
|
||||
`);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {}
|
||||
});
|
||||
|
||||
testConcurrentIfSelected('plan-eng-review-data-model-legitimate-json', async () => {
|
||||
const result = await runSkillTest({
|
||||
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,
|
||||
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. 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 =
|
||||
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);
|
||||
|
||||
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(() => {
|
||||
// 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.
|
||||
planDir = setupPlanEngReviewFixture('skill-e2e-plan-eng-denorm-', `# 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?
|
||||
`);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {}
|
||||
});
|
||||
|
||||
testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => {
|
||||
const result = await runSkillTest({
|
||||
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,
|
||||
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');
|
||||
// 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,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);
|
||||
|
||||
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 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: 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,
|
||||
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.
|
||||
// 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|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 =
|
||||
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, {
|
||||
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'], () => {
|
||||
|
|
|
|||
|
|
@ -1978,3 +1978,56 @@ describe('Bundled browser-skills frontmatter contract', () => {
|
|||
}
|
||||
});
|
||||
});
|
||||
|
||||
// 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.
|
||||
// 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');
|
||||
// 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.
|
||||
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 right-sized diff" bullet', () => {
|
||||
expect(engReview).toContain('Data model exception to "right-sized 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 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 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');
|
||||
expect(engReviewSections).toContain('Parent-field-shadowed-by-child');
|
||||
expect(engReviewSections).toContain('FK deletion strategies');
|
||||
expect(engReviewSections).toContain('Snapshot vs render-live');
|
||||
});
|
||||
|
||||
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 <= 15; i++) {
|
||||
expect(engReview).toMatch(new RegExp(`^${i}\\. \\*\\*`, 'm'));
|
||||
}
|
||||
// And item 16 should NOT exist (the list ends at 15)
|
||||
expect(engReview).not.toMatch(/^16\. \*\*/m);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue