From 1820f58f698f49469e90c60a20774813750c6b76 Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:45:43 -0700 Subject: [PATCH] fix: drop bare "no" as a negation trigger, treat markdown bullets as boundaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex structured review (large-diff gate, Step 11) found 3 more real gaps: - matchesUnnegated's bare "no" trigger over-fired on common positive-framing idioms: "No blocker here: create a separate tier model" and "there is no downside to promoting the payload" both got misread as negated, even though they're endorsements of the recommendation that follows, not rejections. Removed bare "no" from the negation list — "not"/"never"/ "cannot"/the "-n't" family are far more reliable negation signals and don't share this idiom-collision problem. - SENTENCE_END only recognized period-followed-by-capital as a boundary, so a bulleted recommendation like "I would not keep this inline.\n- Separate tier model" (very common LLM output formatting) let the earlier "not" leak into the bullet below it. Added a second alternative: a newline followed by a markdown bullet or numbered-list marker now also counts as a clause boundary. Fixed the boundary-index arithmetic to use the full match length (em[0].length) instead of a bare +1, since this new alternative can match more than one character ("\n- ", "\n1. "). - The minimal-change eval's own hardcoded doesn't-pattern only matched the ASCII apostrophe (`doesn'?t`), missing the curly one — inconsistent with matchesUnnegated's own apostrophe handling a few lines away in the same file. Added 3 regression tests reproducing each directly. bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts test/gen-skill-docs.test.ts — 788 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 24 ++++++++++++++++++++++++ test/helpers/e2e-helpers.ts | 19 +++++++++++++++---- test/skill-e2e-plan.test.ts | 2 +- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index a5d252daf..723e835bb 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -107,6 +107,30 @@ describe('matchesUnnegated', () => { )).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.', diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index c675d0319..0c10bfcb1 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -187,22 +187,33 @@ export function matchesUnnegated(text: string, pattern: RegExp): boolean { // 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). - const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; + // 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 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. - const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)/g; + // 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) { - sentenceStart = em.index + 1; + // 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); if (!NEGATION.test(preceding)) return true; diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 76dfe110b..1883b1109 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -762,7 +762,7 @@ when the verification email link is clicked. const acceptsInlineAddition = 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); + /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: