mirror of https://github.com/garrytan/gstack.git
fix: drop bare "no" as a negation trigger, treat markdown bullets as boundaries
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.
This commit is contained in:
parent
02e312c057
commit
1820f58f69
|
|
@ -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.',
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Reference in New Issue