fix(review): bridge label-separator punctuation in test-evidence negation checks - #3372
Merged
Merged
Conversation
…tion 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.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
10 tasks
Contributor
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 Gittensory is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3372 +/- ##
=======================================
Coverage 93.90% 93.91%
=======================================
Files 284 284
Lines 30607 30608 +1
Branches 11148 11148
=======================================
+ Hits 28743 28744 +1
Misses 1208 1208
Partials 656 656
🚀 New features to boost your workflow:
|
This was referenced Jul 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hasValidationNote's negation-proximity checks required a literal whitespace gap next to the test/validation stem or negation word. 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 that adjacency entirely, so the negation went undetected and the bare stem fell through to the affirmative check instead — letting a PR body that explicitly says validation was not done still satisfy a configured manifest test expectation.src/signals/test-evidence.ts:35treatedValidation: not run/Tests: not runas passing evidence because:was neither a split boundary nor allowed between the test stem and negation.LABEL_SEPARATOR_GAP— a single colon/semicolon/dash (ASCII, en dash, or em dash) with optional trailing whitespace — which stands 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."Tests; not run."previously split into"Tests"and" not run"as independent clauses, letting the bare"Tests"half pass as affirmative on its own).Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥97% coverage of the lines AND branches you changed (aim for 98%+ on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
src/signals/test-evidence.ts) and its unit test. Rannpm run typecheckand the full targeted test suite locally (test/unit/test-evidence.test.ts— 19/19 passing, including 2 new regression tests for this fix — plustest/unit/predicted-gate.test.tsandtest/unit/rules.test.ts, the two consumers ofhasValidationNote, both fully green at 140/140 total). Causality for the new tests was proven by reverting the source fix and confirming the new assertions fail against the prior (merged) logic. The remaining checks (actionlint, workers, MCP, UI, audit) are unaffected by this diff and are left to CI.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository. (N/A — no UI change.)UI Evidence
N/A — no UI/frontend/docs/extension change.
Notes
hasValidationNote(rounds 1-3 landed in fix(review): explain validation and guardrail holds #3304). Verified this fix against every prior round's regression case plus new adversarial cases (a genuine semicolon-joined independent clause must not be falsely bridged, e.g."No documentation issues; tests pass regardless."still correctly resolves totrue) to avoid reintroducing an earlier bug class one level up.