test: add minimal-change counterexample eval, unit-test matchesUnnegated

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

@@ -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 });
+    }
+  });
+});

test/helpers/touchfiles.ts

@@ -92,6 +92,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
   '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<string, 'gate' | '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',
 

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'], () => {