From 09ad6fb1ef917aa51c42c065efd137556cd32128 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 17:37:23 -0700 Subject: [PATCH] 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);