Skip to content

feat: add max/min count thresholds to sequence rules - #1162

Open
theredspoon wants to merge 2 commits into
vale-cli:v3from
theredspoon:feat/sequence-max-min
Open

feat: add max/min count thresholds to sequence rules#1162
theredspoon wants to merge 2 commits into
vale-cli:v3from
theredspoon:feat/sequence-max-min

Conversation

@theredspoon

@theredspoon theredspoon commented Aug 31, 2026

Copy link
Copy Markdown

Closes #1161

Review note: this branch is rebased on #1169 (needed underneath for correctness, see Fix). Only the commit feat: add max/min count thresholds to sequence rules belongs to this PR. Click into it on the Commits tab to see this PR's actual diff. The Files changed tab includes #1169's content too, and will shrink once it merges into v3 and this rebases.

Problem

sequence matches tagged patterns but only alerts once per match. It has no count threshold. occurrence has Max/Min but only matches a raw regex, no tag/upos. Root cause: NewSequence unconditionally narrowed every declared scope to sentence-level, so a paragraph with two real matches split across two sentences stayed silent, since each sentence individually had only one match.

tbhb/vale-ai-tells's VerbTricolonDensity.yml needs exactly this. Its occurrence regex can't require the matched words be verbs. A sequence rule can, and now finally gets a count threshold too.

Fix

Run now tags each sentence of a rule's real declared scope separately (reusing internal/nlp/prose.go's existing per-sentence tagging), instead of tagging the whole block once and inferring boundaries afterward. Sentence membership becomes a direct fact, so a match can't span two sentences by construction.

  • Threshold rules (Max/Min set) keep their real declared scope instead of being narrowed to sentence-level.
  • An undeclared scope on a threshold rule defaults to paragraph plus prose-container scopes, matching plain-rule behavior, via one shared list instead of two hand-copied ones.
  • File.Sentences routes through the same local/remote segmentation dispatch the rest of the codebase already uses.
Why the redesign, not a smaller patch (root cause detail)

Sentence boundaries used to be inferred after tagging a whole block once: compare each word's offset against each sentence's own offset. That broke down for a remote endpoint's unpositioned tokens, which carry no offsets to compare. Tagging each sentence separately makes sentence membership a direct fact instead, determined by which loop iteration produced the word.

Performance

A real but small per-rule cost for local English tagging, 6-8% (see below for the benchmark). A non-English remote endpoint pays up to 2 round-trips per sentence instead of 1; match-walk cost drops from O(N²) per block to a sum of smaller O(N²/k) terms across k sentences.

Full performance reasoning

For a style's sequence rules as a whole, this is a small, consistent cost for local English tagging, not the neutral-to-noise result originally claimed here. The new per-sentence segmentation pass is cached per file, so only the first rule to touch a sentence pays for the actual segmentation work; every other rule sharing that file's cache gets a map-lookup hit instead. That part of the claim holds. What doesn't: BenchmarkSequencePlainRuleSentencesOverhead (internal/check/sequence_bench_test.go), compared with benchstat against a control that calls the same matching logic without ever going through File.Sentences, measures one plain rule against a cold cache at 8.1% slower (16.49µs vs a 15.26µs baseline, p<0.001, n=10). Five plain rules sharing one file's cache still cost 5.7% more per rule on average (5.10µs vs 4.83µs baseline, p=0.02, n=10), not the noise-level difference this section originally claimed. The remainder isn't the segmentation call itself. Cache hits there cost about 19ns (see the next benchmark). It's the fixed per-call cost of Run's new sentence-loop wrapping, which every rule pays whether or not its cache lookup was a hit.

sequenceMatches' match-walk is quadratic in the number of words per call. Splitting a block into sentences turns one large quadratic term into a sum of smaller ones, N²/k instead of for k sentences. Each sentence also pays a small, fixed per-call cost: tokenization and a fresh lookup map. A block of many very short sentences can let that fixed cost outweigh the quadratic saving. Realistic prose doesn't hit this.

Sentence segmentation is cached: a repeated call against the same text hits a map lookup. BenchmarkFileSentencesCache (internal/check/sequence_bench_test.go), compared with benchstat, measures a cold segmentation at 4.93µs against a cached lookup at 19.3ns: a 99.6% reduction, roughly 256x faster (p<0.001, n=10), against a three-sentence fixture. That is well past the ~140x this section originally estimated before a real benchmark existed. The exact multiple depends on the fixture: cache-hit cost is a fixed map lookup, but cold cost scales with how much there is to segment.

A non-English document behind a remote NLP endpoint pays an extra HTTP round-trip, but not a flat doubling. A plain (sentence-scoped) rule now makes one segmentation call and one tagging call per sentence, two round-trips where there was one before. A max/min rule segments its whole scope in one call, then tags each sentence individually: one extra segmentation round-trip per block rather than per sentence, shrinking as a fraction of the total as a block's sentence count grows. Both calls are cached per file, so a style with many sequence rules pays this once per unique sentence, not once per rule. Local English tagging, the common case, never touches this path at all.

Testing

  • cross-sentence aggregation, Max/Min combined
  • cross-sentence boundary guard under both local and remote tagging, including a remote tagger normalizing token text
  • undeclared-scope threshold rule dispatching into headings and list items
  • internal/e2e case (checks/sequence/max) for the density-across-sentences motivating case, end-to-end through a real .vale.ini/style/document
  • TestSequenceActionResolvesSuggestion: a threshold (max/min) rule with a configured action (replace/remove/suggest) now resolves and carries its suggestion, the same as a plain rule already does. This was a real gap the latest rebase surfaced, not part of the original change: extracting the per-sentence match loop into its own matchesIn helper predates resolveFix existing on this codepath at all, so the extraction never called it. Confirmed red without the call, green with it restored.

Full repo suite and -race are clean, including internal/e2e. Confirmed no reintroduction of the concurrency risk sequence was historically excluded from concurrent dispatch for.

Alternative considered: extending occurrence.go instead (rejected)

occurrence could reuse sequence's matching helpers, but it runs concurrently across rules while sequence's tagging cache isn't synchronized. Extending it would touch shared dispatch infrastructure and give Occurrence two structurally different matching modes.

Related

min: N per-token repetition (#899) · scope: paragraph fix (#1124/#1126) · sentenceScope negated-scope double-report fix, split out to #1169 · redundant round-trip elimination for plain rules, explored separately in #1170, closed since it only makes sense as a commit here, not standalone

@theredspoon theredspoon changed the title check(sequence): add max/min count threshold, same as occurrence fix(sequence): count-threshold rules now dispatch at their declared scope, tagging each sentence separately Sep 2, 2026
@theredspoon
theredspoon marked this pull request as ready for review September 2, 2026 01:02
@theredspoon theredspoon changed the title fix(sequence): count-threshold rules now dispatch at their declared scope, tagging each sentence separately feat(sequence): add max/min count thresholds Sep 2, 2026
@theredspoon theredspoon changed the title feat(sequence): add max/min count thresholds feat: add max/min count thresholds to sequence rules Sep 2, 2026
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch 2 times, most recently from 3f86ddb to 5fb16d5 Compare September 2, 2026 02:42
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch 2 times, most recently from 839eab5 to 86405ce Compare September 2, 2026 03:06
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch 2 times, most recently from 2643c6d to 99f0ca0 Compare September 2, 2026 03:49
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch from 99f0ca0 to 0f9a605 Compare September 7, 2026 01:56
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Before this can be merged, please read Vale's contributor license agreement, which is four short points, and reply to this comment with the sentence below. This is once per person, not per pull request.


I have read the CLA and I agree to it


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@theredspoon

Copy link
Copy Markdown
Author

recheck

@theredspoon

Copy link
Copy Markdown
Author

I have read the CLA and I agree to it.

@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch 2 times, most recently from 74099d9 to b84253c Compare September 7, 2026 03:46
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.

Assisted-by: Claude Code
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch from b84253c to 104e13a Compare September 7, 2026 04:02
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#1169 (fixes a sentenceScope bug that review of this feature
found as a real, dispatched double-report).

Assisted-by: Claude Code
@theredspoon
theredspoon force-pushed the feat/sequence-max-min branch from 104e13a to ffb7343 Compare September 7, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sequence has no way to count occurrences of the whole pattern within a scope, only occurrence does, and occurrence has no tag support

1 participant