mirror of https://github.com/garrytan/gstack.git
test: simplify matchesUnnegated, drop its dedicated unit test file
Per PR review feedback: the negation-aware matcher these E2E evals rely on had accumulated real complexity (sentence-boundary scanning, markdown-bullet clause detection, curly-apostrophe contraction matching, idiom exclusions) across four rounds of specialist/adversarial review during this branch's own /ship — each finding was real, but chasing all of them was disproportionate for a periodic-tier, non-gating eval. An occasional missed edge case there just means a rare, visible false read, not a missed real regression. Reverted to a plain fixed-window lookback (60 chars, accounting for the patterns' own internal gaps) with a short negation word list. Kept the generic [a-z]+n't contraction match (can't/won't/aren't) since it's one regex alternative, not meaningful added complexity. Removed test/helpers/e2e-helpers.test.ts — a dedicated unit-test file for a best-effort text-matching heuristic used only by 4 paid, periodic-tier evals was more test infrastructure than the actual ask (3-4 new eval cases) warranted.
This commit is contained in:
David Grant
parent
54a50e4b67
commit
b39da77129
|
|
@ -1,204 +0,0 @@
|
|||
/**
|
||||
* 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('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('recognizes "-n\'t" contractions generically (can\'t, won\'t, aren\'t, hasn\'t)', () => {
|
||||
// A bare `\b n't \b` alternative can never match mid-word (no word
|
||||
// boundary between the letters immediately before "n" and "n" itself),
|
||||
// so only the contractions spelled out explicitly ever matched. Verify
|
||||
// the generic [a-z]+n't pattern actually catches the common ones that
|
||||
// previously fell through silently.
|
||||
expect(matchesUnnegated("This can't promote the payload into columns.", splitPattern)).toBe(false);
|
||||
expect(matchesUnnegated("This won't promote the payload into columns.", splitPattern)).toBe(false);
|
||||
expect(matchesUnnegated("These aren't reasons to promote the payload into columns.", splitPattern)).toBe(false);
|
||||
expect(matchesUnnegated("It hasn't been decided to promote the payload into columns.", splitPattern)).toBe(false);
|
||||
});
|
||||
|
||||
test('recognizes "cannot" as a negation', () => {
|
||||
expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false);
|
||||
});
|
||||
|
||||
test('recognizes a typographic curly apostrophe ("doesn’t") as a negation', () => {
|
||||
// LLM prose commonly renders contractions with U+2019 rather than the
|
||||
// ASCII apostrophe — a plain [a-z]+n't pattern misses this entirely.
|
||||
expect(matchesUnnegated(
|
||||
'This doesn’t need extraction — promote the payload to explicit columns is unnecessary here.',
|
||||
/promote the payload to explicit columns/i,
|
||||
)).toBe(false);
|
||||
});
|
||||
|
||||
test('does not let an abbreviation period (e.g., i.e., etc.) hide an earlier negation', () => {
|
||||
// A bare lastIndexOf('.') treats every period as a sentence break,
|
||||
// including ones inside abbreviations — which would incorrectly cut the
|
||||
// negation lookback window short and hide "not" from the scan.
|
||||
expect(matchesUnnegated(
|
||||
'I would not recommend, e.g., promoting this payload to explicit columns.',
|
||||
/promoting this payload to explicit columns/i,
|
||||
)).toBe(false);
|
||||
});
|
||||
|
||||
test('still treats a real sentence boundary as a boundary (does not over-widen the window)', () => {
|
||||
// Guards against overcorrecting the abbreviation fix into ignoring
|
||||
// genuine sentence breaks — an earlier, unrelated negation must NOT
|
||||
// suppress a later, unnegated positive match.
|
||||
expect(matchesUnnegated(
|
||||
'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.',
|
||||
/promote the payload into explicit columns/i,
|
||||
)).toBe(true);
|
||||
});
|
||||
|
||||
test('does not treat a bare "no" idiom as a negation ("no blocker", "no downside")', () => {
|
||||
// "No blocker here: create a separate tier model" and "there's no
|
||||
// downside to promoting the payload" are positive-framing idioms in
|
||||
// review prose, not negations of the recommendation that follows.
|
||||
expect(matchesUnnegated(
|
||||
'No blocker here: create a separate tier model.',
|
||||
/create[^.]{0,60}separate[^.]{0,20}tier[^.]{0,20}model/i,
|
||||
)).toBe(true);
|
||||
expect(matchesUnnegated(
|
||||
'There is no downside to promoting the payload into explicit columns.',
|
||||
/promoting[^.]{0,60}payload[^.]{0,60}column/i,
|
||||
)).toBe(true);
|
||||
});
|
||||
|
||||
test('treats a newline + markdown bullet as a clause boundary', () => {
|
||||
// "I would not keep this inline.\n- Separate tier model" is common LLM
|
||||
// formatting — without treating the bullet as a boundary, the "not" from
|
||||
// the prose sentence above would leak into the bullet line below it.
|
||||
expect(matchesUnnegated(
|
||||
'I would not keep this inline.\n- Separate subscription tier model',
|
||||
/separate[^.]{0,20}subscription[^.]{0,20}tier[^.]{0,20}model/i,
|
||||
)).toBe(true);
|
||||
});
|
||||
|
||||
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('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. 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();
|
||||
});
|
||||
|
||||
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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -157,65 +157,23 @@ ${focusSentence}`;
|
|||
|
||||
/**
|
||||
* Case-insensitive check for `pattern` in `text` that ignores matches
|
||||
* immediately preceded by a negation word (not/n't/no/never/without/...).
|
||||
* 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 inspects the text just before it for a negation cue.
|
||||
* every match and checks 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.
|
||||
* 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 {
|
||||
// "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.
|
||||
// [a-z]+n't matches any "-n't" contraction generically (can't, won't,
|
||||
// aren't, hasn't, doesn't, don't, wouldn't, ...) — a bare `\b n't \b`
|
||||
// alternative can never match mid-word (there's no word boundary between
|
||||
// the letters immediately before "n" and "n" itself), so it silently
|
||||
// covered nothing beyond the contractions already spelled out explicitly.
|
||||
// [a-z]+n['’]t matches both the ASCII apostrophe and the typographic
|
||||
// curly one (’, U+2019) — LLM prose commonly renders contractions with the
|
||||
// curly form, and this codebase already hit this exact gotcha once before
|
||||
// (see the apostrophe wildcard note in test/spec-template-invariants.test.ts).
|
||||
// Deliberately does NOT include a bare "no" — "no blocker here: create a
|
||||
// separate model" and "no downside to promoting the payload" are common
|
||||
// POSITIVE-framing idioms in review prose, not negations of what follows;
|
||||
// "not"/"never"/"cannot"/the "-n't" family are far more reliable signals.
|
||||
const NEGATION = /\b(not|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i;
|
||||
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);
|
||||
// Real sentence-ending punctuation is followed by whitespace + a capital
|
||||
// letter, or sits at the end of the string — this excludes periods inside
|
||||
// abbreviations ("e.g.", "i.e.", "etc."), decimals, and version strings,
|
||||
// which a bare lastIndexOf('.') would wrongly treat as a sentence break
|
||||
// and use to hide an earlier negation word from the lookback window. A
|
||||
// newline followed by a markdown bullet/numbered-list marker is ALSO a
|
||||
// clause boundary — "I would not keep this inline.\n- Separate tier model"
|
||||
// is common LLM formatting, and without this the "not" from the prose
|
||||
// sentence above would otherwise leak into the bullet below it.
|
||||
const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)|\n\s*(?:[-*+]|\d+[.)])\s/g;
|
||||
let m: RegExpExecArray | null;
|
||||
while ((m = re.exec(text))) {
|
||||
let sentenceStart = 0;
|
||||
let em: RegExpExecArray | null;
|
||||
SENTENCE_END.lastIndex = 0;
|
||||
while ((em = SENTENCE_END.exec(text)) && em.index < m.index) {
|
||||
// em[0].length, not a bare +1 — the bullet-marker alternative above is
|
||||
// multiple characters wide ("\n- ", "\n1. "), so a fixed +1 would leave
|
||||
// part of the marker inside the scanned "preceding" window.
|
||||
sentenceStart = em.index + em[0].length;
|
||||
}
|
||||
const preceding = text.slice(sentenceStart, m.index);
|
||||
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
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue