From 0f054540b4c5ae7a63c2151b8346ae5b0a8e68f6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 22:26:29 -0700 Subject: [PATCH] fix(review): bridge label-separator punctuation in test-evidence negation checks hasValidationNote's proximity checks required a literal whitespace gap next to the test/validation stem or negation word, so a label-style status line that glues its separator directly onto that word with no surrounding space ("Tests: not run.", "Validation; skipped.", "Tests - not run.") broke the adjacency check entirely. The negation went undetected and the bare stem fell through to the affirmative check instead, so a PR body explicitly saying validation was not done could still satisfy a configured manifest test expectation. Introduce LABEL_SEPARATOR_GAP, a single colon/semicolon/dash (ASCII, en dash, or em dash) with optional trailing whitespace, and allow it to stand in for the mandatory whitespace only at the junction touching the stem/negation word itself -- every other gap between filler words stays pure whitespace, so a separator elsewhere in a longer sentence still cannot bridge a negation across unrelated content. Also drop semicolon from the clause-splitting boundary: splitting there severed a stem from its own negation before the proximity checks ever ran, the same failure mode one level up. --- src/signals/test-evidence.ts | 29 ++++++++++++++++++++++------- test/unit/test-evidence.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/signals/test-evidence.ts b/src/signals/test-evidence.ts index 6b74eda343..5f0630761f 100644 --- a/src/signals/test-evidence.ts +++ b/src/signals/test-evidence.ts @@ -28,16 +28,27 @@ export function hasLocalTestEvidence(input: { tests?: string[] | undefined; test // negation by PROXIMITY: a negation word within a few words of a test/validation stem, in either order, // with a shared stem definition so the "is this a test/validation mention at all" question is answered // exactly once. The filler between the negation word and the stem may not cross a clause/sentence boundary -// (a comma/period/exclamation/question mark/semicolon), so an unrelated "not" earlier in the body (e.g. +// (a comma, period, exclamation mark, or question mark), so an unrelated "not" earlier in the body (e.g. // "This is not a breaking change. Tested with npm run test:ci.") cannot suppress a later, unrelated -// affirmative note. +// affirmative note. A colon, semicolon, or dash is deliberately NOT a hard boundary here -- see +// LABEL_SEPARATOR_GAP below. const TEST_STEM = "(?:test(?:ed|s|ing)?|validat(?:ion|ed)|verif(?:y|ied|ying)|manual check|smoke(?:\\s+tests?)?)"; const NEGATION_WORD = "(?:no|not|never|without|skip(?:ped)?|didn't|doesn't|isn't|wasn't|weren't|haven't|hasn't)"; const NEGATION_CONTINUATION = "(?:not|never|failed|failing|skipped|incomplete)"; const SAME_SENTENCE_FILLER_WORD = "[^\\s.,!?;]+"; +// A label-style status line often glues its separator directly onto the negation word or stem with no +// surrounding whitespace ("Tests: not run.", "Validation; skipped.", "Tests - not run."). The plain +// `\s+` gap below would never match across that punctuation, so the negation went undetected and the +// bare "Tests"/"Validation" keyword fell through to the affirmative check instead (#3304, round 4). +// Allow ONE label separator (colon, semicolon, or a hyphen/en-dash/em-dash) with any trailing +// whitespace to stand in for the mandatory whitespace, but only at the junction touching the negation +// word or stem itself -- every other gap between filler words stays pure whitespace, so a label +// separator elsewhere in the sentence still cannot let a negation reach across unrelated content (the +// filler-word bound below already exists for exactly this reason). +const LABEL_SEPARATOR_GAP = "(?:\\s+|[:;\\-\\u2013\\u2014]\\s*)"; -const NEGATES_BEFORE_TEST_STEM = new RegExp(`\\b${NEGATION_WORD}\\b(?:\\s+${SAME_SENTENCE_FILLER_WORD}){0,3}\\s+${TEST_STEM}\\b`, "i"); -const NEGATES_AFTER_TEST_STEM = new RegExp(`\\b${TEST_STEM}\\b(?:\\s+${SAME_SENTENCE_FILLER_WORD}){0,2}\\s+${NEGATION_CONTINUATION}\\b`, "i"); +const NEGATES_BEFORE_TEST_STEM = new RegExp(`\\b${NEGATION_WORD}\\b${LABEL_SEPARATOR_GAP}(?:${SAME_SENTENCE_FILLER_WORD}\\s+){0,3}${TEST_STEM}\\b`, "i"); +const NEGATES_AFTER_TEST_STEM = new RegExp(`\\b${TEST_STEM}\\b${LABEL_SEPARATOR_GAP}(?:${SAME_SENTENCE_FILLER_WORD}\\s+){0,2}${NEGATION_CONTINUATION}\\b`, "i"); // A compound negated adjective with no separating whitespace at all ("untested", "unvalidated", "unverified"). const NEGATES_TEST_STEM_PREFIX = /\bun(?:tested|validated|verified)\b/i; @@ -47,11 +58,15 @@ const AFFIRMATIVE_TEST_MENTION = /\b(test(?:ed|s|ing)?|validation|validated|veri // with real affirmative evidence ("Validated with npm run test:ci.") -- evaluating the negation checks // against the WHOLE body would let the first clause veto the second, discarding real evidence the manifest // gate is specifically trying to detect (#3304, round 3). Split on the same clause-boundary punctuation the -// proximity checks already treat as a hard stop, and require at least one clause to be an affirmative, -// non-negated mention -- so an earlier honest "no tests" disclosure can no longer suppress later evidence. +// proximity checks already treat as a hard stop -- colon/semicolon/dash are excluded here on purpose +// (#3304, round 4): they are typically a label separator glued directly onto the word on either side +// ("Tests: not run."), and splitting on them would sever the stem from its own negation before the +// proximity checks ever run, the same way the round-3 bug worked one level up. Require at least one +// clause to be an affirmative, non-negated mention -- so an earlier honest "no tests" disclosure can no +// longer suppress later evidence. export function hasValidationNote(value: string): boolean { return value - .split(/[.,!?;]+/) + .split(/[.,!?]+/) .some( (clause) => !NEGATES_TEST_STEM_PREFIX.test(clause) && diff --git a/test/unit/test-evidence.test.ts b/test/unit/test-evidence.test.ts index 6cfaac3259..a5b2b711b2 100644 --- a/test/unit/test-evidence.test.ts +++ b/test/unit/test-evidence.test.ts @@ -131,6 +131,38 @@ describe("test evidence helpers", () => { it("still rejects a body whose only test/validation mentions are all negated across clauses", () => { expect(hasValidationNote("No tests run. Not validated. Untested change.")).toBe(false); }); + + // REGRESSION (#3304, round 4): a label-style status line glues its separator directly onto the stem + // or negation word with no surrounding whitespace ("Tests: not run."). The proximity checks previously + // required a literal space next to the stem/negation word, so the colon/semicolon/dash broke that + // adjacency, the negation went undetected, and the bare "Tests"/"Validation" keyword fell through to + // the affirmative check instead. + it("detects a negation glued to the stem or negation word by a colon, semicolon, or dash", () => { + expect(hasValidationNote("Tests: not run.")).toBe(false); + expect(hasValidationNote("Validation: not run.")).toBe(false); + expect(hasValidationNote("Tests: not run")).toBe(false); + expect(hasValidationNote("Validation: Not Run.")).toBe(false); + expect(hasValidationNote("Test: skipped.")).toBe(false); + expect(hasValidationNote("Tests:not run.")).toBe(false); + expect(hasValidationNote("Tests - not run.")).toBe(false); + expect(hasValidationNote("Tests — not run.")).toBe(false); // em dash + expect(hasValidationNote("Tests – not run.")).toBe(false); // en dash + expect(hasValidationNote("Tests; not run.")).toBe(false); + expect(hasValidationNote("Validation; not run.")).toBe(false); + expect(hasValidationNote("Skipped: tests were not run.")).toBe(false); // negation word as the label + }); + + // REGRESSION (#3304, round 4): the label-separator gap must stay bounded to the junction touching the + // stem/negation word -- it must not let a colon/semicolon/dash elsewhere in a longer sentence bridge a + // negation across unrelated filler words, and a genuine colon/semicolon-glued negated clause still + // must not suppress a separate, later real affirmative clause. + it("does not let a label separator elsewhere in the sentence bridge an unrelated negation", () => { + expect(hasValidationNote("No documentation issues; tests pass regardless.")).toBe(true); + expect(hasValidationNote("Not sure if this fully works; tests pass regardless.")).toBe(true); + expect(hasValidationNote("No changes needed here; validated with npm test.")).toBe(true); + expect(hasValidationNote("Tests: not run. Validated with npm run test:ci.")).toBe(true); + expect(hasValidationNote("Tests; not run. Validated with npm run test:ci.")).toBe(true); + }); }); describe("classifyTestCoverage", () => {