diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index 5e61561ea..47701a6e9 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -44,6 +44,24 @@ describe('matchesUnnegated', () => { )).toBe(false); }); + test('ignores a match that is the rejected half of a "rather than" contrast', () => { + // "Keep it on the existing model RATHER THAN promote the payload into + // columns" — the rejected alternative matches the pattern just as + // strongly as a real recommendation would, with no single negation + // word (not/never/without/...) anywhere near it. + expect(matchesUnnegated( + 'Keep this field on the existing model rather than promote the payload into explicit columns.', + splitPattern, + )).toBe(false); + }); + + test('ignores a match that is the rejected half of an "instead of" contrast', () => { + expect(matchesUnnegated( + 'Keep this on the existing model instead of splitting the payload into columns.', + splitPattern, + )).toBe(false); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', @@ -67,8 +85,23 @@ describe('matchesUnnegated', () => { expect(matchesUnnegated('This review has nothing to do with payloads or columns.', splitPattern)).toBe(false); }); + test('ignores a negation far earlier in the same (period-free) sentence', () => { + // The negation-lookback scans to the sentence boundary, not a fixed + // character count — this is the scenario that motivated that design. + // "not" sits well over 20 characters before "extract...payload...columns" + // but is still part of the same sentence (no period between them). + expect(matchesUnnegated( + "I do not think it's worth the added complexity of maintaining a separate table just to extract the payload into columns for these rare cases.", + splitPattern, + )).toBe(false); + }); + test('does not infinite-loop on a zero-width-adjacent pattern', () => { - // Guards the re.lastIndex++ zero-width-match protection. + // Guards the re.lastIndex++ zero-width-match protection. Note: a bare + // quantifier pattern like /x*/ always produces an empty match at index 0 + // with nothing preceding it (so it's trivially "unnegated" and returns + // true immediately) — this test's value is that it terminates at all, + // not that it exercises the negated branch of the loop. const zeroWidthish = /x*/i; expect(() => matchesUnnegated('no x here', zeroWidthish)).not.toThrow(); }); diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index 9b8cb93d6..e09e6b352 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -139,15 +139,34 @@ export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: strin * Plain substring/regex matching can't tell "extract the payload into * columns" from "would NOT extract the payload into columns" — this walks * every match and inspects the text just before it for a negation cue. + * + * The negation lookback scans back to the start of the CURRENT SENTENCE + * (the last `.`/`!`/`?` before the match) rather than a fixed character + * count. A fixed window (originally 20 chars) is too narrow: the patterns + * this is used with have their own internal gaps of up to 80 chars (e.g. + * `(verb)[^.]{0,80}payload[^.]{0,80}column`), so a negation word like "not" + * can legitimately sit further back in the same sentence than a small fixed + * window would see — "I do not think it's worth extracting the payload into + * columns" would otherwise be misread as an unnegated recommendation. + * Scanning to the sentence boundary matches the same period-bounded + * assumption the calling patterns already make. */ export function matchesUnnegated(text: string, pattern: RegExp): boolean { - const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against)\b/i; + // "rather than"/"instead of" catch contrastive phrasing ("add this field + // to the model RATHER THAN create a separate table") — the rejected + // alternative can otherwise match the pattern just as strongly as a real + // recommendation would, with no single negation word anywhere near it. + const NEGATION = /\b(not|n't|no|never|don't|doesn't|didn't|shouldn't|wouldn't|isn't|without|against|rather than|instead of)\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); let m: RegExpExecArray | null; while ((m = re.exec(text))) { - const windowStart = Math.max(0, m.index - 20); - const preceding = text.slice(windowStart, m.index); + const sentenceStart = Math.max( + text.lastIndexOf('.', m.index), + text.lastIndexOf('!', m.index), + text.lastIndexOf('?', m.index), + ) + 1; + const preceding = text.slice(sentenceStart, m.index); if (!NEGATION.test(preceding)) return true; if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches } diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 9bee7c542..1a4dbb7f7 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -760,13 +760,16 @@ Focus specifically on the data model design in the plan. Apply the data model re const discussesTheField = lower.includes('email_verified_at') || lower.includes('verification'); // matchesUnnegated so a CORRECT "I would NOT extract a separate model // here" answer doesn't trip this check just for containing the words. + // Verb list deliberately excludes "add" — "add this new field to the + // existing model" is exactly how a CORRECT (inline) recommendation gets + // phrased, so including it would false-positive on the desired answer. const recommendsSeparateModel = matchesUnnegated( review, - /(extract|split|promote|create)[^.]{0,60}(separate|new|dedicated)[^.]{0,40}(model|table)/i, + /(extract|split|promote|create|introduce|build|spin[- ]?out|break[- ]?out)[^.]{0,60}(separate|new|dedicated)[^.]{0,40}(model|table)/i, ) || matchesUnnegated(review, /separate\s+email\s*verification\s*model/i); const acceptsInlineAddition = - /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i.test(review) || - /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i.test(review) || + matchesUnnegated(review, /(keep|stay|remain|fine|appropriate|correct|reasonable|no need)[^.]{0,60}(inline|as[- ]is|single column|one column|single field)/i) || + matchesUnnegated(review, /(single|one)\s+(trivial\s+)?field[^.]{0,60}(no|without)[^.]{0,40}(independent|separate)/i) || /doesn'?t (need|require|warrant)[^.]{0,40}(a\s+)?(separate|new|dedicated)[^.]{0,40}(model|table)/i.test(review); recordE2E(evalCollector, '/plan-eng-review-data-model-minimal-change', 'Plan Eng Review Data-Model Minimal Change E2E', result, {