fix: a sequence rule with a negated scope double-reports every match - #1169
Open
theredspoon wants to merge 1 commit into
Open
fix: a sequence rule with a negated scope double-reports every match#1169theredspoon wants to merge 1 commit into
theredspoon wants to merge 1 commit into
Conversation
sentenceScope's negation branch was a no-op for a bare negated term: `~list` narrowed to `~list`, itself, via strings.CutPrefix re-adding the same prefix it had just stripped. A negated term never mentions `sentence`, so asksForSentence (scope.go) then skipped every `sentence.*` fragment block for such a rule, and Scope.Matches instead matched both the whole-block copy and its own paragraph wrapper for the same text. One real match dispatched to Run twice, once per block, and produced two identical alerts. The negation branch now AND-s `sentence` in front of the term instead of leaving it untouched, so `~list` narrows to `sentence&~list`, sentences outside a list, the same as every other declared scope already does.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
Run called f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to `sentence`, paying a redundant segmentation pass (a full remote round-trip under a configured non-English endpoint) on text that had already been segmented once, by the same segmenter, to build the block Run was handed. Trusting a rule's declared scope unconditionally to justify skipping that call is unsafe: sentenceScope's negation branch was a real bug (fixed separately in vale-cli#1169) that could otherwise have made this optimization reintroduce a cross-sentence false positive. Gated instead on a structural fact about the block itself: Block.IsSentence reports whether blk's scope was built by doNLP's segmentation loop, the only place that produces a `sentence.`-prefixed scope. Re-segmenting one segmenter's own output with that same segmenter can only ever reproduce it, so the call is skipped exactly when it is truly redundant. Scope.Matches reads the same helper for its own, unrelated `sentence.` check, so the two readings of "is this a sentence block" cannot drift apart. Covered by a new invariant test over the whole scope grammar, a dispatched regression test proving this doesn't reopen the negated-scope false positive vale-cli#1169 fixed, an IsSentence contract test, and a call-count test proving the round-trip savings directly against a mocked remote endpoint.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
Run called f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to `sentence`, paying a redundant segmentation pass (a full remote round-trip under a configured non-English endpoint) on text that had already been segmented once, by the same segmenter, to build the block Run was handed. Trusting a rule's declared scope unconditionally to justify skipping that call is unsafe: sentenceScope's negation branch was a real bug (fixed separately in vale-cli#1169) that could otherwise have made this optimization reintroduce a cross-sentence false positive. Gated instead on a structural fact about the block itself: Block.IsSentence reports whether blk's scope was built by doNLP's segmentation loop, the only place that produces a `sentence.`-prefixed scope. Re-segmenting one segmenter's own output with that same segmenter can only ever reproduce it, so the call is skipped exactly when it is truly redundant. Scope.Matches reads the same helper for its own, unrelated `sentence.` check, so the two readings of "is this a sentence block" cannot drift apart. Covered by a new invariant test over the whole scope grammar, a dispatched regression test proving this doesn't reopen the negated-scope false positive vale-cli#1169 fixed, an IsSentence contract test, and a call-count test proving the round-trip savings directly against a mocked remote endpoint.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
Run called f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to `sentence`, paying a redundant segmentation pass (a full remote round-trip under a configured non-English endpoint) on text that had already been segmented once, by the same segmenter, to build the block Run was handed. Trusting a rule's declared scope unconditionally to justify skipping that call is unsafe: sentenceScope's negation branch was a real bug (fixed separately in vale-cli#1169) that could otherwise have made this optimization reintroduce a cross-sentence false positive. Gated instead on a structural fact about the block itself: Block.IsSentence reports whether blk's scope was built by doNLP's segmentation loop, the only place that produces a `sentence.`-prefixed scope. Re-segmenting one segmenter's own output with that same segmenter can only ever reproduce it, so the call is skipped exactly when it is truly redundant. Scope.Matches reads the same helper for its own, unrelated `sentence.` check, so the two readings of "is this a sentence block" cannot drift apart. Covered by a new invariant test over the whole scope grammar, a dispatched regression test proving this doesn't reopen the negated-scope false positive vale-cli#1169 fixed, an IsSentence contract test, and a call-count test proving the round-trip savings directly against a mocked remote endpoint.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167 (fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit) and vale-cli#1169 (fixes a sentenceScope bug that review of this feature found as a real, dispatched double-report).
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
Run called f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to `sentence`, paying a redundant segmentation pass (a full remote round-trip under a configured non-English endpoint) on text that had already been segmented once, by the same segmenter, to build the block Run was handed. Trusting a rule's declared scope unconditionally to justify skipping that call is unsafe: sentenceScope's negation branch was a real bug (fixed separately in vale-cli#1169) that could otherwise have made this optimization reintroduce a cross-sentence false positive. Gated instead on a structural fact about the block itself: Block.IsSentence reports whether blk's scope was built by doNLP's segmentation loop, the only place that produces a `sentence.`-prefixed scope. Re-segmenting one segmenter's own output with that same segmenter can only ever reproduce it, so the call is skipped exactly when it is truly redundant. Scope.Matches reads the same helper for its own, unrelated `sentence.` check, so the two readings of "is this a sentence block" cannot drift apart. Covered by a new invariant test over the whole scope grammar, a dispatched regression test proving this doesn't reopen the negated-scope false positive vale-cli#1169 fixed, an IsSentence contract test, and a call-count test proving the round-trip savings directly against a mocked remote endpoint.
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
1 participant
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.
Problem
A
sequencerule with a bare negated scope (scope: ~list) reachesRuntwice for the same real match: once via the whole-block copy of the text, once more via its ownparagraph.*wrapper.File.AddAlert's own line/span dedup absorbs this before it reaches a realvalerun's printed output. A user runningvaletoday sees the correct single alert.The bug is still worth fixing directly. It wastes a full extra match-and-tag pass per matching document.
Scope.Matchesalso has a real guarantee: asequencerule declaring any scope should only ever receive single-sentence blocks. This bug silently breaks that guarantee for a negated scope. Other code that relies on it holding, a later optimization forsequence.Run, say, can't do so safely while this stays broken.Root cause
sentenceScope's negation branch was a no-op:~listnarrowed to~list, itself, viastrings.CutPrefix(s, "~")re-adding the same prefix it had just stripped.A negated term never mentions
sentence, soasksForSentence(scope.go) then skips everysentence.*fragment block for such a rule.Scope.Matchesfalls back to matching both the whole-block copy and its ownparagraph.*wrapper, since neither selector was ever narrowed to ask for a sentence fragment specifically.Fix
The negation branch now AND-s
sentencein front of the term instead of leaving it untouched, so~listnarrows tosentence&~list, meaning sentences outside a list, the same as every other declared scope already does.Testing
TestSentenceScope: two updated/added rows pinning the corrected narrowing (~listand a chained~list&text).TestSequenceNegatedScopeDoesNotDoubleReport: dispatches through the real pipeline,Info.ComputeandScope.Matches, not a hand-built block. It collectsRun's own return value directly, which skipsAddAlert's dedup entirely, so the double-dispatch is actually observable here. Verified against the pre-fix behavior directly: 2 alerts for one real match before the fix, 1 after.No
internal/e2ecase: confirmed empirically that a realvalerun's output is identical before and after this fix (AddAlert's dedup already absorbs the duplicate), so an end-to-end case here would pass either way and prove nothing.Full repo suite and
-raceare clean, includinginternal/e2e.Related
Found during review of #1162. Split out here as an independent, single-concern fix, alongside #1167 and #1168.