From a9124bd15b9246015d4269c2eb73ab80129134d1 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:08:11 -0700 Subject: [PATCH] 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'], () => {