diff --git a/cmd/vale/command.go b/cmd/vale/command.go index 871d9bee..c661e69d 100644 --- a/cmd/vale/command.go +++ b/cmd/vale/command.go @@ -258,8 +258,11 @@ func runTag(args []string, _ *core.CLIFlags) error { return err } - out := core.TextToContext( + out, err := core.TextToContext( string(text), &nlp.Info{Lang: args[1], Endpoint: args[2]}) + if err != nil { + return err + } return printJSON(out) } diff --git a/internal/check/scope.go b/internal/check/scope.go index 1e988c24..283d7f5b 100644 --- a/internal/check/scope.go +++ b/internal/check/scope.go @@ -110,7 +110,7 @@ func (s Scope) Matches(blk nlp.Block) bool { // parent block too -- and that is the copy such a rule must see: a // `scope: heading` rule reading a heading one fragment at a time reported // `a.` as a whole heading (#1150). - fragment := strings.HasPrefix(blk.Scope, "sentence.") + fragment := blk.IsSentence() for _, sel := range s.Selectors { if fragment && !asksForSentence(sel) { diff --git a/internal/check/sequence.go b/internal/check/sequence.go index 44faa2de..d3f5bdfa 100644 --- a/internal/check/sequence.go +++ b/internal/check/sequence.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/jdkato/prose/v3/segment" "github.com/jdkato/prose/v3/tag" "github.com/mitchellh/mapstructure" rx "github.com/vale-cli/vale/v3/internal/regex" @@ -90,6 +91,43 @@ type Sequence struct { // Empty means prose's own tagger, which is what every existing rule gets. Model string + // `max`/`min` (`int`): A count threshold on how many times the whole + // token sequence, not one token within it, occurs in the scope. + // + // Same names and meaning as `occurrence`'s: unset (the zero value) keeps + // the existing behavior of one alert per match. Setting either turns the + // rule into a density check, reporting a single alert with the match + // count in its message instead of one alert per match. `min: 2` on a + // single token already covers that token's own repetition; this is for + // repetition of the whole pattern. + // + // A plain `sequence` rule's declared scope is always narrowed to + // sentences (see sentenceScope), since it reads part-of-speech data + // that is tagged a sentence at a time. Setting `max` or `min` skips + // that narrowing instead: Run is then called once per the rule's real + // declared scope -- once per paragraph for `scope: paragraph` -- so + // this counts matches across every sentence of that scope, not just + // one. Run itself tags and walks each of that scope's sentences one at + // a time (see Run), so a single match can never span two of them: "two + // tricolons in two different sentences of the same paragraph" trips + // `max: 1` the same as two tricolons in one sentence would. + // + // An undeclared scope (see NewSequence) reaches a real paragraph plus + // every other prose container -- a heading, list item, blockquote, + // table cell/header/caption, or figure caption (core.ProseContainerScopes) + // -- but, deliberately, nothing narrower than that: a frontmatter key + // (`text.frontmatter.`), a code comment (`text.comment.block` / + // `text.comment.line`), or link/image alt text (`text.attr.alt`) are + // each prose a plain (sentence-scoped) rule's own undeclared scope does + // reach, but a `max`/`min` rule's does not. Aggregating a count needs a + // real, named container to aggregate over -- unlike a plain rule, which + // just wants "every sentence, everywhere" -- and these three are small, + // narrow fragments a style is unlikely to want density-checked on their + // own; naming one explicitly in `scope` still reaches it like any other + // declared scope would. + Max int + Min int + // `exceptions` (`[]string`): Regexes matched against the sentence; a // sequence match that *begins inside* one of their regions is dropped. // @@ -229,7 +267,46 @@ func NewSequence(cfg *core.Config, generic baseCheck, path string) (Sequence, er rule.exceptRe = append(rule.exceptRe, re) } - rule.Definition.Scope = sentenceScope(rule.Definition.Scope) + // A count-threshold rule needs its real declared scope -- a paragraph's + // worth of sentences, not one at a time -- so Max/Min can aggregate + // across all of them. Narrowing to sentences here is what a plain rule + // needs instead: it has no count to aggregate, and narrowing is what + // lets `scope: paragraph` (and an undeclared scope) reach the sentences + // within a block rather than being handed the whole thing at once. + if rule.thresholdSet() { + // `manager.compileCheck` deliberately leaves a `sequence` rule's + // scope unset rather than defaulting it to `text`, the default every + // other check type gets: this rule needs to tell "the author asked + // for nothing in particular" apart from "the author asked for + // `text`" (see its comment). A plain rule reads unset the same way + // sentenceScope(nil) does, as "every sentence, everywhere." + // + // `paragraph` alone reaches a real body paragraph: ast.go wraps + // exactly that kind of block, and only that kind, as + // `paragraph.` (split=true; see lintProse). A heading, list + // item, blockquote, table cell/header, or figure caption is prose + // too -- ast.go segments and tags it the same way a paragraph is -- + // but it is never wrapped that way (split=false, deliberately: see + // #1132), so it has no `paragraph`-qualified sibling block for + // `paragraph` to match instead. Naming each of those other prose + // container scopes here reaches their own whole (undivided) block + // directly, the same way `paragraph` reaches a real paragraph's; + // none of them is a substring of another, and none is ever a + // sentence fragment's own scope, so this cannot also match a + // fragment or double-count a block already matched by `paragraph`. + if len(rule.Definition.Scope) == 0 { + // "paragraph" is this package's own literal -- the split-based + // default every threshold rule falls back to -- but the other + // five names come from core.ProseContainerScopes, the same + // constants internal/lint/ast.go's tagToScope map builds its own + // scope strings from (see there): both read one shared list + // instead of hand-copying it into two. + rule.Definition.Scope = append( + []string{"paragraph"}, core.ProseContainerScopes...) + } + } else { + rule.Definition.Scope = sentenceScope(rule.Definition.Scope) + } rule.filter = rule.literals() return rule, nil @@ -382,6 +459,13 @@ type match struct { func (m match) ok() bool { return len(m.text) > 0 && m.lo >= 0 && m.hi >= m.lo } +// sequenceMatches walks words -- always exactly one sentence's worth, since +// Run tags and calls this once per sentence (see Run) -- looking for the +// rule's token sequence around an anchor occurrence of target. +// +// Because words never spans more than one sentence, running off either end +// of it (wi < 0 on the left, wi >= sizeW on the right) already *is* hitting a +// sentence boundary; nothing further has to check that separately. func sequenceMatches(idx int, chk Sequence, target NLPToken, words []tag.Token, history []int) match { var text []string @@ -609,11 +693,15 @@ func sentenceScope(declared []string) []string { scopes = append(scopes, s) continue } - // Negation applies to the part being excluded, so it stays in front: - // `~list` narrows to sentences outside a list, not to something - // outside `sentence.list`. - if neg, found := strings.CutPrefix(s, "~"); found { - scopes = append(scopes, "~"+neg) + // A bare negated term names only what to exclude and never mentions + // `sentence` itself, so asksForSentence (scope.go) skips every + // `sentence.*` fragment block for it and the rule matched the whole + // unsegmented block instead -- narrowing to `~list` alone was a + // no-op. AND-ing `sentence` in front keeps the exclusion and still + // narrows: `sentence&~list` is "sentences outside a list", which is + // what the rule actually needs. + if strings.HasPrefix(s, "~") { + scopes = append(scopes, "sentence&"+s) continue } // `paragraph` names no block of its own. Splitting wraps every block @@ -634,90 +722,229 @@ func sentenceScope(declared []string) []string { func (s Sequence) Run(blk nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, error) { var alerts []core.Alert - var offset []string - var history []int // Rule the sequence out before tagging, which is the expensive part. - if len(s.filter) > 0 && !containsAny(blk.Lower, s.filter) { + // + // Skipped when `min` is set: that threshold can trip on zero matches, + // and a missing literal is exactly the zero-match case, not a reason to + // exit before the count is known. + if s.Min == 0 && len(s.filter) > 0 && !containsAny(blk.Lower, s.filter) { return nil, nil } - // This is *always* sentence-scoped. - words, terr := f.TokensWith(s.Model, blk.Text) - if terr != nil { - return nil, terr - } - // A remote NLP endpoint returns text and tags only, so we have no offsets // to work from and have to fall back to locating the match by its text. positioned := f.NLP.Endpoint == "" - txt := blk.Text + if idx, tok, ok := s.anchor(); ok { + // Sentence membership used to be inferred after the fact -- tag the + // whole block once, then compare each word's offset against each + // sentence's own offset (see the deleted sentenceIndices) -- which + // broke down for a remote endpoint's unpositioned tokens (see the + // deleted boundarySentenceIndices). Tagging each sentence separately + // instead (see matchesIn) makes sentence membership a direct fact: + // which loop iteration a word came from. A block already segmented + // for tagging or by an earlier rule is not segmented again, since + // sentences are read from the file's shared cache. + // + // blk.IsSentence reports a fact about how blk itself was built, not + // an inference about what any rule declared -- a plain rule's scope + // is *usually* narrowed to `sentence` (see sentenceScope), but that + // narrowing depends on sentenceScope's own correctness, and trusting + // it unconditionally here would reintroduce exactly the cross-sentence + // match this segmentation exists to prevent the moment that narrowing + // is ever wrong. Gating on the block's own scope instead means + // the call is skipped only when it is truly redundant: re-segmenting + // one segmenter's own output, with the same segmenter, can only ever + // reproduce that same output. Every other block still gets the real + // call, whatever the rule declared. + var sentences []segment.Sentence + if !blk.IsSentence() { + var serr error + sentences, serr = f.Sentences(blk.Text) + if serr != nil { + return nil, serr + } + } + if len(sentences) == 0 { + // An empty or otherwise unsegmentable block still has to be + // walked as itself, not skipped outright. Also the fast path for + // a block already known to be one sentence: re-deriving that + // from f.Sentences would just return the same single piece. + sentences = []segment.Sentence{{Text: blk.Text, Start: 0}} + } + + for _, sent := range sentences { + got, err := s.matchesIn(sent, blk, f, idx, tok, positioned) + // got holds whatever this sentence found before a failure, if + // any; matchesIn's own callers never read alerts back out + // alongside a non-nil error (see lintBlockSerial), so keeping it + // here rather than discarding it costs nothing and keeps this + // loop's accumulation uniform regardless of where a failure + // happens. + alerts = append(alerts, got...) + if err != nil { + return alerts, err + } + } + } + + // The single dispatch point for both a rule that never found an anchor + // and one that walked every sentence: thresholdSet's own doc comment + // covers why this must not be duplicated (see its history with + // NewSequence's scope-narrowing guard, the same kind of drift this + // avoids re-opening one level up). + if s.thresholdSet() { + return s.thresholded(alerts), nil + } + + return alerts, nil +} + +// matchesIn finds every alert the rule's token sequence produces within one +// sentence of blk, tagging and walking that sentence's text on its own. +// +// `history` and `offset` track state within this one sentence's walk -- +// which word indices already anchored a match, and which failed candidates' +// text to mask when locating the next successful one in context. Both start +// fresh on every call rather than carrying over from a previous sentence: +// `words` itself restarts from index 0 each time, so a `history` entry from +// an earlier sentence would name an unrelated word here, not merely a stale +// one. +func (s Sequence) matchesIn(sent segment.Sentence, blk nlp.Block, f *core.File, idx int, tok NLPToken, positioned bool) ([]core.Alert, error) { + var alerts []core.Alert + var offset []string + var history []int + + txt := sent.Text + + words, terr := f.TokensWith(s.Model, txt) + if terr != nil { + return nil, terr + } + excluded := s.exceptionSpans(txt) - idx, tok, ok := s.anchor() - if ok { - { - // Each candidate position for the anchor is one possible - // violation. A `pattern` anchor enumerates them by searching the - // text; a tag-only anchor has nothing to search for, so we let - // sequenceMatches walk the words and stop when it runs out. - for _, loc := range s.candidates(txt, tok, len(words)) { - // These are all possible violations in `txt`: - m := sequenceMatches(idx, s, tok, words, history) - history = append(history, m.index) - - if m.ok() { - span, seq := s.locate(txt, words, m, positioned) - if span == nil { - // We matched but cannot say where; reporting a bogus - // span is worse than reporting nothing. - continue - } + // Each candidate position for the anchor is one possible + // violation. A `pattern` anchor enumerates them by searching the + // text; a tag-only anchor has nothing to search for, so we let + // sequenceMatches walk the words and stop when it runs out. + for _, loc := range s.candidates(txt, tok, len(words)) { + // These are all possible violations in `txt`: + m := sequenceMatches(idx, s, tok, words, history) + history = append(history, m.index) + + if m.ok() { + // Located against this sentence's own text, not the whole + // block: a remote endpoint's unpositioned tokens fall back + // to a text search, which otherwise resolved to the first + // occurrence anywhere in the block rather than the one this + // sentence's match actually came from. + span, seq := s.locate(txt, words, m, positioned) + if span == nil { + // We matched but cannot say where; reporting a bogus + // span is worse than reporting nothing. + continue + } - if beginsInside(excluded, span[0]) { - continue - } + if beginsInside(excluded, span[0]) { + continue + } - // When the block knows where it sits in the document, hand - // back an absolute offset. Otherwise the span is - // block-relative and has to be located by searching, which - // resolves every repeat of a sentence to the first one. - absolute := blk.Offset >= 0 - if absolute { - span = []int{blk.Offset + span[0], blk.Offset + span[1]} - } + // Rebase from sentence-relative to block-relative before the + // existing blk.Offset/absolute-position handling applies. + span = []int{span[0] + sent.Start, span[1] + sent.Start} + + // When the block knows where it sits in the document, hand + // back an absolute offset. Otherwise the span is + // block-relative and has to be located by searching, which + // resolves every repeat of a sentence to the first one. + absolute := blk.Offset >= 0 + if absolute { + span = []int{blk.Offset + span[0], blk.Offset + span[1]} + } - action := s.Action - if s.MatchCase && action.Name == "replace" { - action.Params = recase(action.Params, seq) - } + action := s.Action + if s.MatchCase && action.Name == "replace" { + action.Params = recase(action.Params, seq) + } - a := core.Alert{ - Check: s.Name, Severity: s.Level, Link: s.Link, - Span: span, Hide: false, HasByteOffsets: absolute, - Match: seq, Action: action} - - a.Message, a.Description = formatMessages(s.Message, - s.Description, m.text...) - a.Offset = offset - - alerts = append(alerts, a) - offset = []string{} - } else if loc != nil { - converted, err := re2Loc(txt, loc) - if err != nil { - return alerts, err - } - offset = append(offset, converted) - } + a := core.Alert{ + Check: s.Name, Severity: s.Level, Link: s.Link, + Span: span, Hide: false, HasByteOffsets: absolute, + Match: seq, Action: action} + + a.Message, a.Description = formatMessages(s.Message, + s.Description, m.text...) + a.Offset = offset + + alerts = append(alerts, a) + offset = []string{} + } else if loc != nil { + converted, err := re2Loc(txt, loc) + if err != nil { + return alerts, err } + offset = append(offset, converted) } } return alerts, nil } +// thresholdSet reports whether Max or Min opts the rule into count-threshold +// behavior -- the same "is this set" reading `occurrence`'s Max/Min already +// give a raw regex count, where only a positive value counts and a negative +// one is silently treated the same as unset rather than erroring. +// +// Both sites that decide whether Max/Min is set before thresholded ever runs +// -- NewSequence's scope-narrowing guard and Run's dispatch to thresholded -- +// read it through here so they cannot drift out of agreement. thresholded +// itself keeps its own inline `> 0` checks, matching occurrence.go's +// convention, since by the time it runs Run has already gated the call +// through this same predicate. +// Before this, NewSequence checked `== 0` while Run checked `> 0`; `max: -1` +// failed the first (so scope stayed wide, un-narrowed to sentence) but also +// failed the second (so thresholded was never called), landing on a +// combination -- paragraph-wide scope, one alert per match -- that no +// configuration was meant to produce. +func (s Sequence) thresholdSet() bool { + return s.Max > 0 || s.Min > 0 +} + +// thresholded collapses one alert per match into a single density alert, +// the same reading `occurrence`'s `max`/`min` already give a raw regex +// count: fire once, with the match count substituted into the message, when +// the whole pattern (not one token within it) occurs too often or too +// rarely in this scope. +// +// Called only when `Max` or `Min` is set, so every existing rule -- which +// leaves both at their zero value -- returns matches exactly as it always +// has. +func (s Sequence) thresholded(matches []core.Alert) []core.Alert { + count := len(matches) + if !((s.Max > 0 && count > s.Max) || (s.Min > 0 && count < s.Min)) { + return nil + } + + if count == 0 { + // Zero occurrences can itself break a `min` rule. There is no match + // to point at, so, like `occurrence`, mark the first line. + a := core.Alert{ + Check: s.Name, Severity: s.Level, Link: s.Link, + Span: []int{1, 1}, + } + a.Message = core.CondSprintf(s.Message, count) + a.Description = core.CondSprintf(s.Description, count) + return []core.Alert{a} + } + + a := matches[0] + a.Message = core.CondSprintf(s.Message, count) + a.Description = core.CondSprintf(s.Description, count) + return []core.Alert{a} +} + // anchor picks the token the search starts from. // // A `pattern` token is preferred because it can be located in the text diff --git a/internal/check/sequence_bench_test.go b/internal/check/sequence_bench_test.go new file mode 100644 index 00000000..022e349c --- /dev/null +++ b/internal/check/sequence_bench_test.go @@ -0,0 +1,43 @@ +package check + +import ( + "testing" + + "github.com/vale-cli/vale/v3/internal/core" + "github.com/vale-cli/vale/v3/internal/nlp" +) + +// A plain (sentence-scoped) rule's Run call against a block Info.Compute +// already built as one segmenter piece -- the common case, and the one this +// PR's optimization targets. Local English tagging never touches a remote +// endpoint either way, so this isolates whatever local cost the skipped +// f.Sentences call itself carried (a map lookup into TokenCache.sentences +// after the first call, since the cache is keyed by exact text and this +// benchmark reuses the same block every iteration). +func BenchmarkSequenceRunPlainRuleSentenceBlock(b *testing.B) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Bench.WidgetArrived", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Bench.WidgetArrived") + if err != nil { + b.Fatalf("building rule: %v", err) + } + + text := "I bought a widget that arrived promptly, said the courier." + f := &core.File{NLP: nlp.Info{Segmentation: true}} + blk := nlp.NewLinedBlock("", text, "sentence.text.md", 1) + + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, rerr := rule.Run(blk, f, testConfig()); rerr != nil { + b.Fatalf("running rule: %v", rerr) + } + } +} diff --git a/internal/check/sequence_test.go b/internal/check/sequence_test.go index 6bd5ec12..5d2b256c 100644 --- a/internal/check/sequence_test.go +++ b/internal/check/sequence_test.go @@ -1,8 +1,14 @@ package check import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" "testing" + "github.com/jdkato/prose/v3/tag" + "github.com/vale-cli/vale/v3/internal/core" "github.com/vale-cli/vale/v3/internal/nlp" ) @@ -54,6 +60,700 @@ func TestSequenceMatchesWholeTokens(t *testing.T) { } } +// runScoped runs rule the way the real linter dispatches a `sequence` check: +// through the same block splitting (nlp.Info.Compute) and scope matching +// (Scope.Matches) the linter itself uses, instead of handing text to Run +// directly. +// +// A block built by hand and passed straight to Run bypasses that dispatch +// entirely -- Run sees whatever text the test wrote, not what the rule's own +// declared scope would actually receive in production. That distinction is +// the whole point here: a `max`/`min` rule's declared scope decides whether +// Run is called once per sentence or once per paragraph, and only routing +// through the real scope-matching logic can tell the two apart. +func runScoped(t *testing.T, rule Sequence, text string) []core.Alert { + t.Helper() + + f := &core.File{NLP: nlp.Info{Segmentation: true, Splitting: true}} + paragraph := nlp.NewLinedBlock("", text, "text.md", 1) + + blocks, err := f.NLP.Compute(¶graph, true) + if err != nil { + t.Fatalf("computing blocks: %v", err) + } + + scope := NewScope(rule.Fields().Scope) + + var alerts []core.Alert + for _, blk := range blocks { + if !scope.Matches(blk) { + continue + } + got, rerr := rule.Run(blk, f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + alerts = append(alerts, got...) + } + + return alerts +} + +// runScopedAs is runScoped generalized to the block kind under test. +// +// A threshold rule with an undeclared scope must still dispatch into a +// heading, a list item, or a table cell -- not just a body paragraph -- +// since ast.go builds each of those with split=false (see #1132): a heading +// or a list item is prose, but it is never a "paragraph" block. selector is +// the block's own scope, as ast.go would build it (e.g. "text.heading.h1.md", +// "text.list.md", "text.table.cell.md"); split mirrors ast.go's own choice +// for that block kind (false for all three). +func runScopedAs(t *testing.T, rule Sequence, text, selector string, split bool) []core.Alert { + t.Helper() + + f := &core.File{NLP: nlp.Info{Segmentation: true, Splitting: true}} + blk := nlp.NewLinedBlock("", text, selector, 1) + + blocks, err := f.NLP.Compute(&blk, split) + if err != nil { + t.Fatalf("computing blocks: %v", err) + } + + scope := NewScope(rule.Fields().Scope) + + var alerts []core.Alert + for _, b := range blocks { + if !scope.Matches(b) { + continue + } + got, rerr := rule.Run(b, f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + alerts = append(alerts, got...) + } + + return alerts +} + +// runScopedRemote is runScoped, but dispatched under a remote NLP endpoint -- +// the file carries Endpoint and Lang, the same as a real remotely-tagged +// file would, so every block reaches Run with positioned == false. +func runScopedRemote(t *testing.T, rule Sequence, text, endpoint, lang string) []core.Alert { + t.Helper() + + f := &core.File{NLP: nlp.Info{ + Segmentation: true, Splitting: true, Endpoint: endpoint, Lang: lang, + }} + paragraph := nlp.NewLinedBlock("", text, "text.md", 1) + + blocks, err := f.NLP.Compute(¶graph, true) + if err != nil { + t.Fatalf("computing blocks: %v", err) + } + + scope := NewScope(rule.Fields().Scope) + + var alerts []core.Alert + for _, blk := range blocks { + if !scope.Matches(blk) { + continue + } + got, rerr := rule.Run(blk, f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + alerts = append(alerts, got...) + } + + return alerts +} + +// `max` turns repeated matches of the whole pattern into a single density +// alert, reading "more than N tricolons in this paragraph" instead of one +// alert per tricolon -- the same threshold `occurrence` already applies to a +// raw regex count, but here counting a tag-aware sequence match instead. +func TestSequenceMaxCountsWholePatternOccurrences(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.Tricolon", + "level": "error", + "message": "Too many tricolons (found %d).", + "scope": []string{"paragraph"}, + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ"}, + map[string]interface{}{"tag": ",", "skip": 2}, + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ", "skip": 1}, + map[string]interface{}{"tag": ",", "skip": 2}, + map[string]interface{}{"tag": "CC"}, + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ", "skip": 1}, + }, + }, "Test.Tricolon") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + cases := []struct { + name string + text string + wantAlerts int + wantMessage string + }{ + { + "exactly one tricolon stays under max", + "It validates the payload, transforms the record, and writes the result.", + 0, "", + }, + { + "two tricolons trip max", + "It validates the payload, transforms the record, and writes the result. " + + "It parses the header, checks the signature, and rejects the request.", + 1, "Too many tricolons (found 2).", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + alerts := runScoped(t, rule, c.text) + if len(alerts) != c.wantAlerts { + t.Fatalf("%q produced %d alerts, want %d", c.text, len(alerts), c.wantAlerts) + } + if c.wantAlerts > 0 && alerts[0].Message != c.wantMessage { + t.Errorf("message = %q, want %q", alerts[0].Message, c.wantMessage) + } + }) + } +} + +// Without `max`/`min` set, a `sequence` rule alerts once per match exactly +// as it always has -- the zero value must not change existing behavior. +func TestSequenceWithoutMaxMinAlertsPerMatch(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.TricolonUnbounded", + "level": "error", + "message": "Tricolon found.", + "scope": []string{"paragraph"}, + "tokens": []interface{}{ + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ"}, + map[string]interface{}{"tag": ",", "skip": 2}, + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ", "skip": 1}, + map[string]interface{}{"tag": ",", "skip": 2}, + map[string]interface{}{"tag": "CC"}, + map[string]interface{}{"tag": "VB|VBD|VBG|VBN|VBP|VBZ", "skip": 1}, + }, + }, "Test.TricolonUnbounded") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "It validates the payload, transforms the record, and writes the result. " + + "It parses the header, checks the signature, and rejects the request." + + f := &core.File{NLP: nlp.Info{}} + alerts, rerr := rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != 2 { + t.Fatalf("produced %d alerts, want 2 (one per match, max/min unset)", len(alerts)) + } + for _, a := range alerts { + if a.Message != "Tricolon found." { + t.Errorf("message = %q, want the unformatted per-match message", a.Message) + } + } +} + +// `min` can trip on zero matches, which has no span to point at -- the same +// document-scoped fallback `occurrence` uses for its own zero case. +func TestSequenceMinOnZeroMatches(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.RequireTwo", + "level": "error", + "message": "Expected at least two, found %d.", + "scope": []string{"paragraph"}, + "min": 2, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.RequireTwo") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "This paragraph never mentions the term at all." + + f := &core.File{NLP: nlp.Info{}} + alerts, rerr := rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != 1 { + t.Fatalf("produced %d alerts, want 1", len(alerts)) + } + if alerts[0].Message != "Expected at least two, found 0." { + t.Errorf("message = %q, want the zero-count message", alerts[0].Message) + } + if len(alerts[0].Span) != 2 || alerts[0].Span[0] != 1 || alerts[0].Span[1] != 1 { + t.Errorf("span = %v, want the document-scoped [1, 1] fallback", alerts[0].Span) + } +} + +// A `max`-set rule's declared scope names a real block -- a paragraph, not +// the sentence sequence.go currently forces every rule into. Two matches +// split across two sentences of the same paragraph must trip `max: 1` the +// same as two matches in one sentence would; today they don't, because each +// sentence gets its own Run call with no shared count between them. +func TestSequenceMaxAggregatesAcrossSentences(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.Widget", + "level": "error", + "message": "Too many widgets (found %d).", + "scope": []string{"paragraph"}, + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.Widget") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + alerts := runScoped(t, rule, text) + if len(alerts) != 1 { + t.Fatalf("produced %d alerts, want 1 (one density alert for the paragraph)", len(alerts)) + } + if alerts[0].Message != "Too many widgets (found 2)." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Too many widgets (found 2).") + } +} + +// A threshold rule that names no `scope` at all still has to dispatch. +// `manager.compileCheck` deliberately leaves a `sequence` rule's scope unset +// rather than defaulting it to `text` -- the rule needs to tell "nothing +// declared" apart from an explicit `text` -- so NewSequence is the only place +// left to give an unset threshold rule a real scope. Before that default was +// added, the rule's declared scope stayed empty, and an empty selector set +// never matches any block: `max`/`min` was silently never enforced for any +// rule that did not name `scope: paragraph` (or similar) itself. +func TestSequenceMaxWithoutDeclaredScopeStillDispatches(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetNoScope", + "level": "error", + "message": "Too many widgets (found %d).", + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetNoScope") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + alerts := runScoped(t, rule, text) + if len(alerts) != 1 { + t.Fatalf("produced %d alerts, want 1 (rule must dispatch with no declared scope)", len(alerts)) + } + if alerts[0].Message != "Too many widgets (found 2)." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Too many widgets (found 2).") + } +} + +// `max` and `min` combine the same way `occurrence`'s do: either threshold +// alone can trip the rule. Both need the same real, declared-scope count +// `max` alone needs -- a rule can't apply either bound correctly while still +// counting one sentence at a time. +func TestSequenceMaxAndMinTogether(t *testing.T) { + newRule := func(t *testing.T) Sequence { + t.Helper() + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetBounds", + "level": "error", + "message": "Expected 2-3 widgets, found %d.", + "scope": []string{"paragraph"}, + "max": 3, + "min": 2, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetBounds") + if err != nil { + t.Fatalf("building rule: %v", err) + } + return rule + } + + cases := []struct { + name string + text string + wantAlerts int + wantMessage string + }{ + { + "below min, split across sentences", + "The first widget arrived today. Nothing else happened.", + 1, "Expected 2-3 widgets, found 1.", + }, + { + "within bounds, split across sentences", + "The first widget arrived today. The second widget arrived yesterday.", + 0, "", + }, + { + "above max, split across sentences", + "The first widget and second widget arrived today. " + + "The third widget and fourth widget arrived yesterday.", + 1, "Expected 2-3 widgets, found 4.", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + alerts := runScoped(t, newRule(t), c.text) + if len(alerts) != c.wantAlerts { + t.Fatalf("%q produced %d alerts, want %d", c.text, len(alerts), c.wantAlerts) + } + if c.wantAlerts > 0 && alerts[0].Message != c.wantMessage { + t.Errorf("message = %q, want %q", alerts[0].Message, c.wantMessage) + } + }) + } +} + +// `min`'s at-threshold case is silent, the same way `max`'s already is: a +// count that exactly meets the floor is not "too few." That has to hold for +// the rule's real declared scope -- here a paragraph -- not just for +// whichever single sentence happens to be under test. +func TestSequenceMinSilentAtExactThreshold(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetFloor", + "level": "error", + "message": "Expected at least two widgets, found %d.", + "scope": []string{"paragraph"}, + "min": 2, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetFloor") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + alerts := runScoped(t, rule, text) + if len(alerts) != 0 { + t.Fatalf("produced %d alerts, want 0 (2 widgets exactly meets min: 2)", len(alerts)) + } +} + +// Skipping the sentence-out-early check when `min` is set has to survive past +// a single sentence too: a paragraph where no sentence contains the required +// literal must still report exactly one zero-count alert for the whole +// paragraph -- not silently drop every sentence for lacking the word, and not +// report one alert per sentence either. +func TestSequenceMinReportsAbsentLiteralOncePerParagraph(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.RequireWidgetMention", + "level": "error", + "message": "Expected at least one widget, found %d.", + "scope": []string{"paragraph"}, + "min": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.RequireWidgetMention") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "This paragraph never mentions the term at all. Neither does this second sentence." + + alerts := runScoped(t, rule, text) + if len(alerts) != 1 { + t.Fatalf("produced %d alerts, want 1 (one zero-count alert for the whole paragraph)", len(alerts)) + } + if alerts[0].Message != "Expected at least one widget, found 0." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Expected at least one widget, found 0.") + } +} + +// The match-walk itself has no notion of a sentence boundary: `Run` tags a +// block a sentence at a time, but hands `sequenceMatches` one flat token +// slice with no marker between sentences. Called directly on a hand-built +// block, the way this test does deliberately, that already reaches a block +// spanning two sentences -- runScoped can't get here today, since a plain +// rule's scope narrows to `sentence` and dispatches one sentence at a time, +// but a paragraph-scoped Max/Min rule will hand Run exactly this kind of +// multi-sentence block once the narrowing is relaxed for it. A `skip` window +// only checks word distance, so the last word of one sentence and the first +// of the next can satisfy it as if they were never apart. +func TestSequenceRejectsMatchSpanningSentenceBoundary(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrived", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrived") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + cases := []struct { + name string + text string + want int + }{ + { + "same sentence: match completes", + "I bought a widget that arrived promptly.", + 1, + }, + { + "across a sentence boundary: match must not complete", + "I bought a widget. Arrived promptly, said the courier.", + 0, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f := &core.File{NLP: nlp.Info{}} + alerts, rerr := rule.Run(nlp.NewBlock(c.text, c.text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != c.want { + t.Errorf("%q produced %d alerts, want %d", c.text, len(alerts), c.want) + } + }) + } +} + +// A remote NLP endpoint's tokens carry no real offsets into the source -- +// Start is always 0 in what it returns (see nlp.TextToTokens) -- so the +// boundary guard cannot lean on word.Start there. sentenceIndices instead +// locates each word by its text, which has to work identically whether the +// tokens came from the local tagger or a remote one. This mocks a `/tag` +// endpoint that reproduces that shape (real text and tags, zeroed offsets) +// to make sure the guard still holds over it. +func TestSequenceRejectsMatchSpanningSentenceBoundaryOverRemoteEndpoint(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedRemote", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedRemote") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Tag the text with the same local tagger the non-remote tests use, + // so the words and tags are real -- then drop the offsets, the one + // thing an actual remote endpoint's response does not carry. + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + remote := make([]tag.Token, len(local)) + for i, tok := range local { + remote[i] = tag.Token{Text: tok.Text, Tag: tok.Tag, Start: 0} + } + + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + })) + defer server.Close() + + cases := []struct { + name string + text string + want int + }{ + { + "same sentence: match completes", + "I bought a widget that arrived promptly.", + 1, + }, + { + "across a sentence boundary: match must not complete", + "I bought a widget. Arrived promptly, said the courier.", + 0, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "en"}} + alerts, rerr := rule.Run(nlp.NewBlock(c.text, c.text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != c.want { + t.Errorf("%q produced %d alerts, want %d", c.text, len(alerts), c.want) + } + }) + } +} + +// A remote tagger's normalization does not always leave a token's text +// unchanged, even when it leaves the token count and order alone. Locating +// tokens by searching txt for their (possibly rewritten) text broke exactly +// here: normalizing the pronoun "I" to "i" made a naive search match the +// letter "i" embedded inside "widget" instead of the real word, stalling the +// search cursor mid-word; the next lookup then matched inside a word in the +// *next* sentence, jumping the cursor across the real sentence boundary and +// mis-assigning every word after it -- silently dropping the guard for a +// genuine violation. boundarySentenceIndices aligns by ordinal position +// instead, which does not depend on the token text matching at all, only the +// count -- this reproduces the normalization and confirms the guard holds. +func TestSequenceRejectsMatchSpanningSentenceBoundaryOverRemoteEndpointNormalization(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedRemoteNormalized", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedRemoteNormalized") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + remote := make([]tag.Token, len(local)) + for i, tok := range local { + text := tok.Text + if text == "I" { + // The normalization that broke text-based location: a + // single-letter token whose lowercased form recurs inside an + // unrelated word ("widget") later in the same text. + text = "i" + } + remote[i] = tag.Token{Text: text, Tag: tok.Tag, Start: 0} + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + })) + defer server.Close() + + text := "I bought a widget. Arrived promptly, said the courier." + + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "en"}} + alerts, rerr := rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != 0 { + t.Errorf("%q produced %d alerts, want 0 (normalization must not defeat the boundary guard)", + text, len(alerts)) + } +} + +// Under the old design, a remote tokenizer that disagreed with the local one +// on token count (not just text) left boundarySentenceIndices unable to +// align the two lists by position, so it gave up and returned nil -- +// which crossesSentence read as "reject nothing," letting a genuine +// cross-sentence match complete. That was a documented trade-off of +// inferring sentence membership from a whole block's flat token list after +// the fact. +// +// Per-sentence tagging removes the scenario entirely rather than handling it +// better: each sentence is tagged by its own call, using only that +// sentence's own text, so there is no shared, block-wide token list for a +// remote tokenizer to disagree with the local one *about* in the first +// place. A dropped token still changes what that one sentence's own tagging +// looks like, but it cannot let a match reach across into a different +// sentence's words -- those simply are never in the same slice. This test +// now asserts the guard holds even under a token-dropping mock endpoint, +// where the old design would have silently let it through. +func TestSequenceRemoteTokenCountMismatchStillGuardsBoundary(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedRemoteMismatch", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedRemoteMismatch") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + // Drop the last token of whichever text this call received. Under + // the new per-sentence design that is always just one sentence's + // text, so this reproduces a per-sentence tokenizer disagreement, + // not a whole-block one. + remote := make([]tag.Token, 0, len(local)-1) + for _, tok := range local[:len(local)-1] { + remote = append(remote, tag.Token{Text: tok.Text, Tag: tok.Tag, Start: 0}) + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + })) + defer server.Close() + + text := "I bought a widget. Arrived promptly, said the courier." + + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "en"}} + alerts, rerr := rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + // Each sentence is tagged on its own text, so a dropped token never + // creates a merged, misaligned view spanning both sentences: the + // cross-sentence match must still be rejected. + if len(alerts) != 0 { + t.Errorf("%q produced %d alerts, want 0 (per-sentence tagging must not let a "+ + "per-sentence tokenizer disagreement open a cross-sentence match)", + text, len(alerts)) + } +} + func testConfig() *core.Config { return &core.Config{WordTemplate: wordTemplate} } @@ -71,7 +771,15 @@ func TestSentenceScope(t *testing.T) { {"a block scope is narrowed", []string{"list"}, []string{"sentence.list"}}, {"already a sentence scope", []string{"sentence"}, []string{"sentence"}}, {"already narrowed", []string{"sentence.list"}, []string{"sentence.list"}}, - {"negation stays in front", []string{"~list"}, []string{"~list"}}, + // A bare negated term never mentions `sentence`, so asksForSentence + // (scope.go) skipped every `sentence.*` fragment block for it and the + // rule matched the whole unsegmented block instead: `~list` alone left + // `s` unchanged. `sentence&~list` still excludes list items, but only + // within sentence-fragment blocks. + {"negation is AND-ed with sentence", + []string{"~list"}, []string{"sentence&~list"}}, + {"a chained negation is AND-ed the same way", + []string{"~list&text"}, []string{"sentence&~list&text"}}, {"several at once", []string{"heading", "list"}, []string{"sentence.heading", "sentence.list"}}, @@ -103,6 +811,65 @@ func TestSentenceScope(t *testing.T) { } } +// The real, dispatched consequence of the negation bug above: a plain rule +// with a bare negated scope matched the same real-world sentence through two +// different blocks at once. `~list` narrowed to nothing, so Scope.Matches +// treated the rule as if it had no scope at all and matched both +// `paragraph.text.md` and its own whole-block copy `text.md` -- the same +// underlying text, dispatched to Run twice, once for each block. One real +// match produced two identical alerts. +// +// Dispatched the way the real linter dispatches a `sequence` check: through +// the same block splitting (nlp.Info.Compute) and scope matching +// (Scope.Matches) it uses, instead of handing text to Run directly. A block +// built by hand and passed straight to Run bypasses that dispatch entirely, +// so it cannot see this bug at all. +func TestSequenceNegatedScopeDoesNotDoubleReport(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedNegatedScope", + "level": "error", + "ignorecase": true, + "message": "matched", + "scope": []string{"~list"}, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedNegatedScope") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "I bought a widget that arrived promptly, said the courier." + + f := &core.File{NLP: nlp.Info{Segmentation: true, Splitting: true}} + paragraph := nlp.NewLinedBlock("", text, "text.md", 1) + + blocks, cerr := f.NLP.Compute(¶graph, true) + if cerr != nil { + t.Fatalf("computing blocks: %v", cerr) + } + + scope := NewScope(rule.Fields().Scope) + + var alerts []core.Alert + for _, blk := range blocks { + if !scope.Matches(blk) { + continue + } + got, rerr := rule.Run(blk, f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + alerts = append(alerts, got...) + } + + if len(alerts) != 1 { + t.Errorf("produced %d alerts for one real match, want exactly 1", len(alerts)) + } +} + // The narrowed scope has to match a block that is actually built. A paragraph's // sentences arrive as `sentence.text.`, so a rule scoped to `paragraph` // must match that or it reports nothing at all. @@ -136,6 +903,219 @@ func TestSequenceParagraphScopeMatchesParagraphSentences(t *testing.T) { } } +// This is the invariant every other test here assumes rather than checks: +// whatever a `sequence` rule declares, every block Scope.Matches ever hands +// it is one sentence a segmenter already produced. Stated once, over the +// whole scope grammar, instead of pinned indirectly by whichever declared +// scopes happen to have their own test. +func TestScopeMatchesOnlyHandsSequenceSentenceBlocks(t *testing.T) { + declaredScopes := [][]string{ + nil, + {"paragraph"}, + {"heading"}, + {"text"}, + {"sentence"}, + {"sentence.list"}, + {"~list"}, + {"~code"}, + {"~list", "text"}, + {"heading", "~heading.h1"}, + } + + text := "One sentence here. Two sentences here, for good measure." + + for _, declared := range declaredScopes { + t.Run(strings.Join(declared, "&"), func(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.ScopeGrammar", + "level": "error", + "message": "matched", + "scope": declared, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "sentence"}, + }, + }, "Test.ScopeGrammar") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + f := &core.File{NLP: nlp.Info{Segmentation: true, Splitting: true}} + paragraph := nlp.NewLinedBlock("", text, "text.md", 1) + + blocks, cerr := f.NLP.Compute(¶graph, true) + if cerr != nil { + t.Fatalf("computing blocks: %v", cerr) + } + + // Not every declared scope matches a block this flat + // paragraph builds (there is no heading or list block here) -- + // that is fine. The invariant under test is narrower and holds + // regardless: whatever Scope.Matches does hand back, it is + // always a sentence block. + scope := NewScope(rule.Fields().Scope) + for _, blk := range blocks { + if !scope.Matches(blk) { + continue + } + if !blk.IsSentence() { + t.Errorf("scope %v matched block %q (scope %q), which is not a sentence block", + declared, blk.Text, blk.Scope) + } + } + }) + } +} + +// A negated-scope plain rule (`scope: ~list`) must still reject a +// cross-sentence match under real dispatch, with this optimization active. +// +// This does not actually discriminate blk.IsSentence's gating from the old +// unconditional f.Sentences call: sentenceScope correctly narrows `~list` to +// `sentence&~list`, so by the time Scope.Matches ever hands Run a block for +// this rule, that block is always sentence-scoped -- IsSentence() is true, +// and skipping f.Sentences on a genuine single sentence returns the same +// single sentence back that a real call would have. Reverting only the gate +// leaves this test passing, confirmed directly. What it does verify, and is +// worth keeping for, is that sentenceScope's narrowing and Run's gating +// compose correctly for the scenario each was written against: one at the +// dispatch layer, one inside Run. A future change to either that broke that +// composition would fail here even though neither one's own tests would +// necessarily catch it in isolation. +func TestSequenceRoundTripSkipDoesNotReintroduceNegatedScopeFalsePositive(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedNegatedScopeRoundTrip", + "level": "error", + "ignorecase": true, + "message": "matched", + "scope": []string{"~list"}, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedNegatedScopeRoundTrip") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "I bought a widget. Arrived promptly, said the courier." + alerts := runScoped(t, rule, text) + if len(alerts) != 0 { + t.Errorf("produced %d alerts, want 0 (match spans two sentences)", len(alerts)) + } +} + +// This is the actual round-trip saving this optimization exists for, proven +// by counting real requests rather than asserting it indirectly: a plain +// (sentence-scoped) rule dispatched through the real pipeline over a +// multi-sentence paragraph must not add any /segment call of its own beyond +// the one Info.Compute already made to build the blocks in the first place. +// +// This also has to prove the optimization does not silently break ordinary +// matching in the process: the rule's genuine match lives entirely within +// the first dispatched sentence, so if skipping f.Sentences on a +// blk.IsSentence() block ever handed matchesIn something other than that +// exact sentence's own text, the match would stop resolving and this test +// would catch that as a wrong alert count, not just a wrong call count. +// +// Verified against the actual pre-fix behavior, not just asserted: before +// this change, this same scenario made 2 total /segment calls (1 from +// Compute, 1 more from the sentence block whose text still contains the +// rule's required literal -- the other dispatched sentence lacks that +// literal and exits before ever reaching f.Sentences, via the unrelated +// early-exit optimization above; a rule without a usable literal prefilter +// would pay one extra call per dispatched sentence instead of one). +// TokenCache.Sentences' cache did not prevent this. It is keyed by exact +// text. Each sentence's text differs from the paragraph's, and from each +// other's, so each one was a real, first-time cache miss. +func TestSequencePlainRuleAddsNoSegmentCallsBeyondCompute(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedNoExtraCalls", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedNoExtraCalls") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + var segmentCalls int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/segment": + segmentCalls++ + text := r.URL.Query().Get("text") + result := nlp.SegmentResult{Sents: strings.Split(text, ". ")} + if encErr := json.NewEncoder(w).Encode(result); encErr != nil { + t.Errorf("encoding mock /segment response: %v", encErr) + } + case "/tag": + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + remote := make([]tag.Token, len(local)) + for i, tok := range local { + remote[i] = tag.Token{Text: tok.Text, Tag: tok.Tag, Start: 0} + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + default: + t.Errorf("unexpected request path: %s", r.URL.Path) + } + })) + defer server.Close() + + text := "I bought a widget that arrived promptly. It shipped yesterday." + f := &core.File{NLP: nlp.Info{ + Segmentation: true, Splitting: true, Endpoint: server.URL, Lang: "id", + }} + paragraph := nlp.NewLinedBlock("", text, "text.md", 1) + + blocks, cerr := f.NLP.Compute(¶graph, true) + if cerr != nil { + t.Fatalf("computing blocks: %v", cerr) + } + if segmentCalls != 1 { + t.Fatalf("test setup: Info.Compute itself made %d /segment calls, want 1", segmentCalls) + } + + scope := NewScope(rule.Fields().Scope) + dispatched := 0 + var alerts []core.Alert + for _, blk := range blocks { + if !scope.Matches(blk) { + continue + } + dispatched++ + got, rerr := rule.Run(blk, f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + alerts = append(alerts, got...) + } + if dispatched != 2 { + t.Fatalf("test setup: rule dispatched into %d blocks, want 2 (one per sentence)", dispatched) + } + if len(alerts) != 1 { + t.Errorf("produced %d alerts, want exactly 1 (the genuine match in the first sentence "+ + "must still resolve correctly under the gated path)", len(alerts)) + } + + if segmentCalls != 1 { + t.Errorf("total /segment calls = %d, want 1 (Run must add none beyond Compute's own)", + segmentCalls) + } +} + // A token found inside its `skip` window satisfies that window alone: the // tokens after it still have to hold. This rule once fired on any "the ... // noun" tail, whether or not a past-tense verb followed. @@ -410,3 +1390,509 @@ func TestSequenceMinConsecutive(t *testing.T) { }) } } + +// --------------------------------------------------------------------------- +// RED-phase regression tests for the new per-sentence tagging design (see the +// architectural review that replaces sentenceIndices/boundarySentenceIndices/ +// crossesSentence/sentIdx). Each test below reproduces a specific bug found +// in the old, count-threshold cross-sentence guard across three review +// rounds, and must keep passing under the new design without reproducing it. +// --------------------------------------------------------------------------- + +// A threshold rule's undeclared scope defaults to `["paragraph"]` (see +// NewSequence). "paragraph" names no block of its own, the same way it +// doesn't for a plain rule's own scope-narrowing (see sentenceScope's own +// comment) -- but a heading or list item is prose, not a paragraph: ast.go +// builds both with split=false, precisely so that a `scope: paragraph` rule +// does not reach them (#1132). Real dispatch must still deliver a threshold +// rule with no declared scope into a heading, exactly as a plain +// (sentence-scoped) `sequence` rule already does. Today it does not: the +// declared scope literally reads "paragraph", and neither the heading's own +// block (`text.heading.h1.md`) nor its sentence fragments satisfy that +// selector, so the rule is never called at all and the violation goes +// unreported, silently. +func TestSequenceMaxWithoutDeclaredScopeDispatchesIntoHeading(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetHeadingNoScope", + "level": "error", + "message": "Too many widgets (found %d).", + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetHeadingNoScope") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + // split=false: exactly how ast.go builds a heading (lintProse(f, b, + // lines, false), see internal/lint/ast.go). + alerts := runScopedAs(t, rule, text, "text.heading.h1.md", false) + if len(alerts) != 1 { + t.Fatalf("heading: produced %d alerts, want 1 (an undeclared scope "+ + "must still reach a heading, see #1132)", len(alerts)) + } + if alerts[0].Message != "Too many widgets (found 2)." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Too many widgets (found 2).") + } +} + +// Same gap, for a list item -- also built with split=false (ast.go maps +// `li` to `text.list`), and also not a paragraph. +func TestSequenceMaxWithoutDeclaredScopeDispatchesIntoListItem(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetListNoScope", + "level": "error", + "message": "Too many widgets (found %d).", + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetListNoScope") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + alerts := runScopedAs(t, rule, text, "text.list.md", false) + if len(alerts) != 1 { + t.Fatalf("list item: produced %d alerts, want 1 (an undeclared scope "+ + "must still reach a list item, see #1132)", len(alerts)) + } + if alerts[0].Message != "Too many widgets (found 2)." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Too many widgets (found 2).") + } +} + +// Same gap again, for a table cell -- ast.go maps `td`/`th` to +// `text.table.cell`/`text.table.header`, also split=false, also not a +// paragraph. +func TestSequenceMaxWithoutDeclaredScopeDispatchesIntoTableCell(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetTableNoScope", + "level": "error", + "message": "Too many widgets (found %d).", + "max": 1, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + }, + }, "Test.WidgetTableNoScope") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "The first widget arrived today. The second widget arrived yesterday." + + alerts := runScopedAs(t, rule, text, "text.table.cell.md", false) + if len(alerts) != 1 { + t.Fatalf("table cell: produced %d alerts, want 1 (an undeclared scope "+ + "must still reach a table cell, see #1132)", len(alerts)) + } + if alerts[0].Message != "Too many widgets (found 2)." { + t.Errorf("message = %q, want %q", alerts[0].Message, "Too many widgets (found 2).") + } +} + +// A remote tagger's normalization is not always length-preserving: +// contracting "do" + "n't" into a single "don't" token, exactly like a real +// tagger would, changes the *total token count* of the sentence it occurs +// in -- not just the text of one token, the way case-folding "I" to "i" +// does. boundarySentenceIndices' realignment (see its doc comment) trusts an +// equal token count between a fresh local retag and the remote list as +// proof the two line up position-for-position; a merge like this breaks +// that equality even though nothing at all is wrong with the remote +// tagger's segmentation. When the counts disagree, boundarySentenceIndices +// gives up and returns nil, which crossesSentence reads as "reject +// nothing" -- so the boundary guard is silently disabled for the entire +// block, and a real cross-sentence match completes. +// +// The new per-sentence design must not reproduce this: each sentence is its +// own call, so which call produced a token is a fact, not something +// recovered by comparing counts -- a token-count-changing normalization +// elsewhere in the paragraph has no way to affect it. +func TestSequenceRemoteNormalizationMergingTokensDoesNotDefeatGuard(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedMergeNorm", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedMergeNorm") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + // Reproduces two ordinary normalizations at once: "I" is lowercased to + // "i" (same token count, as the existing normalization test already + // covers), and "do"+"n't" is merged into a single "don't" token (changes + // the token count, which the existing tests do not cover). + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + var remote []tag.Token + for i := 0; i < len(local); i++ { + tok := local[i] + txt := tok.Text + if txt == "I" { + txt = "i" + } + if txt == "do" && i+1 < len(local) && local[i+1].Text == "n't" { + remote = append(remote, tag.Token{Text: "don't", Tag: tok.Tag, Start: 0}) + i++ + continue + } + remote = append(remote, tag.Token{Text: txt, Tag: tok.Tag, Start: 0}) + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + })) + defer server.Close() + + cases := []struct { + name string + text string + want int + }{ + { + "cross-sentence: match must not complete, even though a same-length " + + "normalization elsewhere ('I'->'i') and a count-changing one " + + "('do'+\"n't\"->\"don't\") both occur in the same text", + "I don't want the widget. Arrived promptly, said the courier.", + 0, + }, + { + "same sentence: a genuine match must still be counted despite the " + + "same normalizations occurring earlier in the text", + "I don't wait; the widget arrived promptly.", + 1, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "en"}} + alerts, rerr := rule.Run(nlp.NewBlock(c.text, c.text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != c.want { + t.Errorf("%q produced %d alerts, want %d", c.text, len(alerts), c.want) + } + }) + } +} + +// File.Sentences (and TokenCache.Sentences beneath it) must route through +// the same segmentation dispatch Info.Compute already uses: local Punkt for +// English, the endpoint's own `/segment` response for non-English text under +// a configured remote endpoint (see Info.Compute). Today it does not -- +// TokenCache.Sentences always calls punktSegmenter().Segment(text) directly, +// ignoring f.NLP entirely, so a `sequence` rule's cross-sentence guard reads +// sentence boundaries from an English-trained local segmenter even when the +// file is configured with a non-English remote endpoint that would have +// reported different boundaries. +// +// This mocks a `/segment` endpoint that deliberately disagrees with local +// Punkt's segmentation of the same text (merging what Punkt splits into two +// sentences into one), so the two are distinguishable: if f.Sentences +// reflects the mock, the dispatch changed; if it still reflects local +// Punkt's own split, it did not. +func TestFileSentencesRoutesThroughEndpointDispatchForNonEnglish(t *testing.T) { + text := "Ini kalimat pertama. Ini kalimat kedua." + + // Sanity check on the test's own premise: local Punkt splits this into + // two sentences (confirmed separately), so returning exactly one from + // the mock is a real, detectable disagreement -- not a coincidence. + local, lerr := (&core.File{}).Sentences(text) + if lerr != nil { + t.Fatalf("segmenting locally: %v", lerr) + } + if len(local) != 2 { + t.Fatalf("test setup: want local Punkt to split %q into 2 sentences, got %d", + text, len(local)) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + result := nlp.SegmentResult{Sents: []string{text}} + if err := json.NewEncoder(w).Encode(result); err != nil { + t.Errorf("encoding mock /segment response: %v", err) + } + })) + defer server.Close() + + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "id"}} + got, gerr := f.Sentences(text) + if gerr != nil { + t.Fatalf("segmenting via endpoint: %v", gerr) + } + + if len(got) != 1 { + t.Errorf("f.Sentences returned %d sentences, want 1 (the mocked "+ + "non-English endpoint's /segment response) -- File.Sentences must "+ + "route through the same endpoint dispatch Info.Compute uses for "+ + "non-English text under a configured remote endpoint, not always "+ + "local Punkt", len(got)) + } +} + +// End-to-end companion to TestFileSentencesRoutesThroughEndpointDispatchForNonEnglish: +// that test confirms File.Sentences itself routes through the endpoint +// dispatch; this confirms a real sequence rule's own cross-sentence guard, +// reached through Run, actually reflects it too, now that Run calls +// f.Sentences directly as part of its main loop (see Run) rather than +// through the doomed sentenceIndices detour. +// +// Both /tag and /segment are mocked. The /segment mock deliberately +// disagrees with local Punkt, merging what Punkt splits into two sentences +// into one -- so the same text produces a different outcome depending on +// which segmenter answers: under local Punkt's own two-sentence split the +// match crosses a boundary and is rejected (see +// TestSequenceRejectsMatchSpanningSentenceBoundary); under the mocked +// endpoint's one-sentence view there is no boundary to cross, and the match +// completes. +func TestSequenceRunUsesEndpointSegmentationForNonEnglish(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedSegmentRouting", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedSegmentRouting") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + text := "I bought a widget. Arrived promptly, said the courier." + + // Sanity check on the test's own premise: local Punkt splits this into + // two sentences, so the mocked /segment response below (one sentence) + // is a real, detectable disagreement, not a coincidence. + local, lerr := (&core.File{}).Sentences(text) + if lerr != nil { + t.Fatalf("segmenting locally: %v", lerr) + } + if len(local) != 2 { + t.Fatalf("test setup: want local Punkt to split %q into 2 sentences, got %d", + text, len(local)) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/segment": + result := nlp.SegmentResult{Sents: []string{r.URL.Query().Get("text")}} + if encErr := json.NewEncoder(w).Encode(result); encErr != nil { + t.Errorf("encoding mock /segment response: %v", encErr) + } + case "/tag": + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + remote := make([]tag.Token, len(local)) + for i, tok := range local { + remote[i] = tag.Token{Text: tok.Text, Tag: tok.Tag, Start: 0} + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + default: + t.Errorf("unexpected request path: %s", r.URL.Path) + } + })) + defer server.Close() + + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "id"}} + alerts, rerr := rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + if rerr != nil { + t.Fatalf("running rule: %v", rerr) + } + if len(alerts) != 1 { + t.Errorf("%q produced %d alerts, want 1 -- Run must read sentence "+ + "boundaries from the configured non-English remote endpoint's own "+ + "/segment response (which merges this into one sentence here), not "+ + "always local Punkt's (which splits it into two and would reject "+ + "the match)", text, len(alerts)) + } +} + +// SegmentWith used to panic when a configured remote endpoint's /segment +// request failed -- a network error, timeout, or malformed response -- which +// crashed the whole vale process rather than reporting a normal lint error: +// there is no recover() anywhere in Vale, unlike the tagging path, which +// already threads a real error end-to-end (textToTokensWith -> +// TokenCache.TokensWith -> File.TokensWith -> Run's existing terr handling). +// This mocks a /segment endpoint that fails -- a non-2xx status with a +// malformed (non-JSON) body, which doSegment's json.Unmarshal cannot parse +// -- and confirms Run now returns a normal error the same way it already +// does for a tagging failure, instead of panicking. +func TestSequenceRunReturnsErrorOnSegmentEndpointFailure(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetArrivedSegmentFailure", + "level": "error", + "ignorecase": true, + "message": "matched", + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetArrivedSegmentFailure") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // A malformed, non-JSON /segment response: doSegment's + // json.Unmarshal fails on this the same way it would on a real + // endpoint's timeout page or other non-JSON error body. + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("not json")) + })) + defer server.Close() + + // A non-English language, so Run reaches SegmentWith's remote branch + // (see usesRemoteSegmentation) rather than local Punkt. + text := "I bought a widget. Arrived promptly, said the courier." + f := &core.File{NLP: nlp.Info{Endpoint: server.URL, Lang: "id"}} + + var alerts []core.Alert + var rerr error + func() { + defer func() { + if p := recover(); p != nil { + t.Fatalf("Run panicked instead of returning an error: %v", p) + } + }() + alerts, rerr = rule.Run(nlp.NewBlock(text, text, "text"), f, testConfig()) + }() + + if rerr == nil { + t.Fatalf("Run returned a nil error for a failed /segment request, want a non-nil error") + } + if alerts != nil { + t.Errorf("alerts = %v, want nil alongside the error", alerts) + } +} + +// Span-offset correctness is the highest-risk part of the new design: a +// sentence-relative span found while tagging one sentence at a time has to +// be rebased -- first onto the paragraph's own text (add the sentence's +// offset within it), then onto the document (blk.Offset, unchanged) -- to +// end up as the correct absolute span in the final alert. Get any of that +// wrong and a real violation either mislocates or, as reproduced here, +// disappears. +// +// This is deliberately end-to-end: a two-paragraph document dispatched +// through real scope matching, under a remote (unpositioned) endpoint, where +// the only genuine violation sits in the second sentence of the second +// paragraph. Today, a `max`/`min` rule tags and locates the *whole paragraph* +// in one pass; a remote endpoint's tokens carry no offsets (Start is always +// 0), so `locate` falls back to searching the whole paragraph's text for the +// matched words -- and that search always resolves to the *first* occurrence +// of that text in the paragraph, not necessarily the one the match actually +// came from. An exception region placed over an identical but excluded +// occurrence earlier in the paragraph turns that into an observable bug: the +// genuine, later match's own span is computed as if it were the earlier, +// excluded one, so it is wrongly dropped as beginning inside the exception +// too -- the real violation vanishes instead of being reported with the +// correct span. +// +// Under the new design, each sentence is tagged and located on its own text, +// so a same-text-but-excluded earlier sentence cannot contaminate a later +// sentence's own, correctly-rebased span. +func TestSequenceSpanRebasedCorrectlyAcrossParagraphsAndSentences(t *testing.T) { + rule, err := NewSequence(testConfig(), baseCheck{ + "extends": "sequence", + "name": "Test.WidgetSpanRebase", + "level": "error", + "message": "Expected at least two, found %d.", + "min": 2, + "exceptions": []interface{}{`^[^,]+,`}, + "tokens": []interface{}{ + map[string]interface{}{"pattern": "widget"}, + map[string]interface{}{"pattern": "arrived", "skip": 1}, + }, + }, "Test.WidgetSpanRebase") + if err != nil { + t.Fatalf("building rule: %v", err) + } + + para1 := "This warm-up paragraph never mentions the target term at all, " + + "so it should be entirely inert." + // Sentence 1 of para2 contains a "widget arrived" that begins inside the + // exception region (up to its first comma) and must stay excluded. + // Sentence 2 contains the one genuine, un-excepted match; it is the only + // alert that should survive into the final result. + para2Sent1 := "Nothing about a widget arrived here, so ignore it." + para2Sent2 := "The real widget arrived at noon." + para2 := para2Sent1 + " " + para2Sent2 + doc := para1 + "\n\n" + para2 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + local, terr := nlp.TextToTokens(r.URL.Query().Get("text"), nil) + if terr != nil { + t.Errorf("tagging mock request text: %v", terr) + return + } + remote := make([]tag.Token, len(local)) + for i, tok := range local { + remote[i] = tag.Token{Text: tok.Text, Tag: tok.Tag, Start: 0} + } + if encErr := json.NewEncoder(w).Encode(nlp.TagResult{Tokens: remote}); encErr != nil { + t.Errorf("encoding mock /tag response: %v", encErr) + } + })) + defer server.Close() + + alerts := runScopedRemote(t, rule, doc, server.URL, "en") + + // The correct absolute span: the second, real "widget arrived" sits in + // para2's second sentence, at its position within the whole document. + wantStart := strings.LastIndex(doc, "widget arrived") + wantEnd := wantStart + len("widget arrived") + + var found *core.Alert + for i := range alerts { + if alerts[i].Message == "Expected at least two, found 1." { + found = &alerts[i] + } + } + if found == nil { + t.Fatalf("no alert with message %q among %d alerts (got messages: %v) -- "+ + "the genuine match in paragraph 2's second sentence must not be dropped", + "Expected at least two, found 1.", len(alerts), alertMessages(alerts)) + } + if len(found.Span) != 2 || found.Span[0] != wantStart || found.Span[1] != wantEnd { + t.Errorf("span = %v, want [%d, %d] (the real match in para2's second "+ + "sentence, not a span carried over from the excluded, textually "+ + "identical occurrence in para2's first sentence)", + found.Span, wantStart, wantEnd) + } +} + +func alertMessages(alerts []core.Alert) []string { + out := make([]string, len(alerts)) + for i, a := range alerts { + out[i] = a.Message + } + return out +} diff --git a/internal/core/file.go b/internal/core/file.go index c7a16288..948b4049 100755 --- a/internal/core/file.go +++ b/internal/core/file.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" + "github.com/jdkato/prose/v3/segment" "github.com/jdkato/prose/v3/summarize" "github.com/jdkato/prose/v3/tag" @@ -225,6 +226,34 @@ func (f *File) TokensWith(model, text string) ([]tag.Token, error) { return cache.TokensWith(model, text, &f.NLP) } +// Sentences returns the sentence spans of text -- each one's byte offset +// included -- computed once per document however many rules ask for it. +// +// Segmentation does not depend on a tagger model, unlike TokensWith, so this +// always reads the default model's cache regardless of which model a rule +// names; a second, model-keyed cache of the same segmentation would just +// repeat it once per model for no reason. +// +// Routed through f.NLP the same way TokensWith is, so a file configured with +// a non-English remote endpoint gets that endpoint's own sentence +// boundaries, not always local Punkt's (see nlp.SegmentWith). +// +// A non-nil error means segmentation itself failed -- a remote endpoint's +// request errored -- the same as TokensWith already reports for tagging. +func (f *File) Sentences(text string) ([]segment.Sentence, error) { + if f.tags == nil { + f.tags = map[string]*nlp.TokenCache{} + } + + cache, ok := f.tags[""] + if !ok { + cache = &nlp.TokenCache{} + f.tags[""] = cache + } + + return cache.Sentences(text, &f.NLP) +} + // StartBlock resets the per-block alert state: the masked contexts, and the // counts of what was masked into them. func (f *File) StartBlock() { diff --git a/internal/core/util.go b/internal/core/util.go index 9a5f96b2..61a636a5 100755 --- a/internal/core/util.go +++ b/internal/core/util.go @@ -11,6 +11,34 @@ import ( "github.com/vale-cli/vale/v3/internal/nlp" ) +// Prose container scope families that lintProse's own segmentation reaches +// besides a real body paragraph: a heading, list item, blockquote, table +// cell/header/caption, or figure caption. Each is prose -- tagged and +// segmented the same way a paragraph is -- but, unlike a paragraph, it is +// never wrapped as `paragraph.` (see #1132): a selector naming the +// family directly is what reaches its own whole block instead. +// +// internal/lint's ast.go builds these blocks (its tagToScope map, and the +// heading case beside it) and internal/check's sequence.go needs the same +// family names for an undeclared `max`/`min` rule's default scope (see +// NewSequence). `check` cannot import `lint` -- the dependency runs the +// other way -- but both already import `core`, so the names live here once, +// read by both, rather than as two hand-copied lists a comment merely asked +// to be kept in sync. +const ( + ScopeHeading = "heading" + ScopeList = "list" + ScopeTable = "table" + ScopeBlockquote = "blockquote" + ScopeFigure = "figure" +) + +// ProseContainerScopes lists the constants above together, for a caller that +// wants all of them at once. +var ProseContainerScopes = []string{ + ScopeHeading, ScopeList, ScopeTable, ScopeBlockquote, ScopeFigure, +} + var defaultIgnoreDirectories = []string{ "node_modules", ".git", } @@ -265,14 +293,19 @@ func SplitLines(data []byte, atEOF bool) (adv int, token []byte, err error) { // return 0, nil, nil } -func TextToContext(text string, meta *nlp.Info) []nlp.TaggedWord { +func TextToContext(text string, meta *nlp.Info) ([]nlp.TaggedWord, error) { context := []nlp.TaggedWord{} for idx, line := range strings.Split(text, "\n") { plain := stripMarkdown(line) + toks, err := nlp.TextToTokens(plain, meta) + if err != nil { + return nil, err + } + pos := 0 - for _, tok := range nlp.TextToTokens(plain, meta) { + for _, tok := range toks { if strings.TrimSpace(tok.Text) != "" { s := strings.Index(line[pos:], tok.Text) + len(line[:pos]) if !StringInSlice(tok.Tag, []string{"''", "``"}) { @@ -288,7 +321,7 @@ func TextToContext(text string, meta *nlp.Info) []nlp.TaggedWord { } } - return context + return context, nil } func ReplaceAllStringSubmatchFunc(re *regexp.Regexp, str string, repl func([]string) string) string { diff --git a/internal/core/util_test.go b/internal/core/util_test.go index c2e47e1b..deafef21 100755 --- a/internal/core/util_test.go +++ b/internal/core/util_test.go @@ -1,11 +1,14 @@ package core import ( + "net/http" + "net/http/httptest" "os" "path/filepath" "reflect" "testing" + "github.com/vale-cli/vale/v3/internal/nlp" "github.com/vale-cli/vale/v3/internal/system" ) @@ -39,6 +42,39 @@ func TestFormatFromExt(t *testing.T) { } } +// TextToContext is the production caller of nlp.TextToTokens (reached from +// the `tag` CLI command, cmd/vale/command.go's runTag) -- it used to panic +// when a configured remote endpoint's /tag request failed, crashing the +// whole vale process instead of letting runTag's already-existing `if err +// != nil { return err }` handling report it normally, the same way +// Info.Compute's fix reused lintProse's own pre-existing error handling. +// This confirms TextToContext now returns a clean error instead. +func TestTextToContextReturnsErrorOnTagEndpointFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Tokens":[]}`)) + })) + defer server.Close() + + var out []nlp.TaggedWord + var err error + func() { + defer func() { + if p := recover(); p != nil { + t.Fatalf("TextToContext panicked instead of returning an error: %v", p) + } + }() + out, err = TextToContext("some text", &nlp.Info{Lang: "id", Endpoint: server.URL}) + }() + + if err == nil { + t.Fatalf("TextToContext returned a nil error for a failed /tag request, want a non-nil error") + } + if out != nil { + t.Errorf("context = %v, want nil alongside the error", out) + } +} + func TestPrepText(t *testing.T) { rawToPrepped := map[string]string{ "foo\r\nbar": "foo\nbar", diff --git a/internal/lint/ast.go b/internal/lint/ast.go index bf7c691e..09eaafbf 100644 --- a/internal/lint/ast.go +++ b/internal/lint/ast.go @@ -45,13 +45,18 @@ var inlineToScope = map[string]string{ "tt": "code", } +// tagToScope's values are built from core.ProseContainerScopes' own family +// names (core.ScopeTable and so on) rather than repeating them as literal +// strings, so a rename or removal there is a compile error here instead of a +// silent drift -- the same family names internal/check's sequence.go reads +// from the same constants for an undeclared `max`/`min` rule's default scope. var tagToScope = map[string]string{ - "th": "text.table.header", - "td": "text.table.cell", - "caption": "text.table.caption", - "li": "text.list", - "blockquote": "text.blockquote", - "figcaption": "text.figure.caption", + "th": "text." + core.ScopeTable + ".header", + "td": "text." + core.ScopeTable + ".cell", + "caption": "text." + core.ScopeTable + ".caption", + "li": "text." + core.ScopeList, + "blockquote": "text." + core.ScopeBlockquote, + "figcaption": "text." + core.ScopeFigure + ".caption", } func (l *Linter) lintHTMLTokens(f *core.File, raw []byte, offset int) error { //nolint:unparam @@ -298,12 +303,12 @@ func (l *Linter) lintScope(f *core.File, state *walker, txt string) error { for _, tag := range state.tagHistory { scope, match := tagToScope[tag] if (match && !core.StringInSlice(tag, inlineTags)) || heading.MatchString(tag) { - if scope == "text.blockquote" || scope == "text.list" { + if scope == "text."+core.ScopeBlockquote || scope == "text."+core.ScopeList { f.Summary.WriteString(txt + "\n\n") } if !match { - scope = "text.heading." + tag + scope = "text." + core.ScopeHeading + "." + tag } f.Metrics[strings.TrimPrefix(scope, "text.")]++ diff --git a/internal/nlp/http.go b/internal/nlp/http.go index 5f61d74e..e4e0fa61 100644 --- a/internal/nlp/http.go +++ b/internal/nlp/http.go @@ -2,6 +2,7 @@ package nlp import ( "encoding/json" + "fmt" "io" "net/http" "net/url" @@ -17,6 +18,15 @@ type TagResult struct { Tokens []tag.Token } +// post sends the request and returns its body, but only for a successful +// (2xx) response. +// +// Without this check, a remote endpoint returning e.g. `500 {"sents":[]}` -- +// an error status with a technically-valid-but-degenerate JSON body -- was +// decoded exactly as if it had succeeded: doSegment or pos would hand back a +// zero-value result and a nil error, and a caller reading that as "no +// sentences" or "no tokens" rather than "the request failed" would silently +// carry on with wrong data instead of surfacing the real problem. func post(url string) ([]byte, error) { var body []byte @@ -31,6 +41,10 @@ func post(url string) ([]byte, error) { return body, err } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("nlp: request to %s failed: %s", url, resp.Status) + } + return body, nil } diff --git a/internal/nlp/http_test.go b/internal/nlp/http_test.go new file mode 100644 index 00000000..3a33c7b8 --- /dev/null +++ b/internal/nlp/http_test.go @@ -0,0 +1,114 @@ +package nlp + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/jdkato/prose/v3/tag" +) + +// A remote endpoint returning a non-2xx status with an otherwise +// well-formed JSON body -- e.g. `500 {"sents":[]}` -- must not be read as a +// successful, empty result. Without a status check, json.Unmarshal has +// nothing to fail on: the caller gets a nil error and a zero-value result, +// indistinguishable from "the endpoint really has nothing to report" -- +// silently wrong instead of a surfaced failure. +func TestPostRejectsNonTwoXXStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"sents":[]}`)) + })) + defer server.Close() + + body, err := post(server.URL) + if err == nil { + t.Fatalf("post returned a nil error for a %d response with a valid JSON body", + http.StatusInternalServerError) + } + if body != nil { + t.Errorf("body = %q, want nil alongside the error", body) + } +} + +// post is the shared transport both doSegment (/segment) and pos (/tag) call +// through, so a status check there has to cover both -- this confirms it +// does for /segment. +func TestDoSegmentReturnsErrorOnNonTwoXXStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Sents":[]}`)) + })) + defer server.Close() + + if _, err := doSegment("some text", "id", server.URL); err == nil { + t.Fatalf("doSegment returned a nil error for a %d /segment response", + http.StatusInternalServerError) + } +} + +// Same check for /tag, the tagging counterpart to /segment. +func TestPosReturnsErrorOnNonTwoXXStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Tokens":[]}`)) + })) + defer server.Close() + + if _, err := pos("some text", "id", server.URL); err == nil { + t.Fatalf("pos returned a nil error for a %d /tag response", + http.StatusInternalServerError) + } +} + +// TextToTokens used to panic when a configured remote endpoint's /tag +// request failed -- a network error, a non-2xx status, a malformed response +// -- which crashed the whole vale process. Its only production caller, +// TextToContext (internal/core/util.go, reached from the `tag` CLI command), +// already had somewhere sensible to route an error: this confirms +// TextToTokens itself now returns one instead of panicking, matching the +// same fix already made to Info.Compute (see +// TestComputeReturnsErrorOnSegmentEndpointFailure in provider_test.go). +func TestTextToTokensReturnsErrorOnTagEndpointFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Tokens":[]}`)) + })) + defer server.Close() + + var toks []tag.Token + var err error + func() { + defer func() { + if p := recover(); p != nil { + t.Fatalf("TextToTokens panicked instead of returning an error: %v", p) + } + }() + toks, err = TextToTokens("some text", &Info{Lang: "id", Endpoint: server.URL}) + }() + + if err == nil { + t.Fatalf("TextToTokens returned a nil error for a failed /tag request, want a non-nil error") + } + if toks != nil { + t.Errorf("tokens = %v, want nil alongside the error", toks) + } +} + +// Control: an ordinary 2xx response must still be read normally. +func TestPostSucceedsOnTwoXXStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"sents":["ok"]}`)) + })) + defer server.Close() + + body, err := post(server.URL) + if err != nil { + t.Fatalf("post returned an error for a 200 response: %v", err) + } + if !strings.Contains(string(body), "ok") { + t.Errorf("body = %q, want it to contain the response payload", body) + } +} diff --git a/internal/nlp/prose.go b/internal/nlp/prose.go index 9868e7e2..0f37d166 100644 --- a/internal/nlp/prose.go +++ b/internal/nlp/prose.go @@ -3,6 +3,7 @@ package nlp import ( "fmt" "os" + "strings" "sync" "github.com/jdkato/prose/v3/segment" @@ -166,23 +167,81 @@ func tagTextWith(model, text string) ([]tag.Token, error) { return tokens, nil } +// SegmentWith splits text into sentences, making the same local-vs-remote +// choice Info.Compute already makes for structural paragraph splitting (see +// usesRemoteSegmentation in provider.go): local Punkt for English text, or +// when info names no remote endpoint at all, and otherwise the endpoint's own +// `/segment` response for non-English text. +// +// Local Punkt's own Sentence values already carry accurate offsets (see +// segment.Sentence). A remote `/segment` response does not -- it is text +// only -- so each returned piece is located by searching text for it, +// advancing a cursor past every earlier piece so a sentence that recurs +// verbatim resolves to its own occurrence rather than always the first (the +// same technique offsetOf, in provider.go, uses for structural splitting). +// +// A remote endpoint's request can fail -- a network error, a timeout, a +// non-JSON body -- so this reports that as a real error rather than +// panicking: there is no recover() anywhere in Vale, so a panic here would +// crash the whole run instead of surfacing as one file's lint error. +func SegmentWith(text string, info *Info) ([]segment.Sentence, error) { + if !usesRemoteSegmentation(info) { + return punktSegmenter().Segment(text), nil + } + + ret, err := doSegment(text, info.Lang, info.Endpoint) + if err != nil { + return nil, err + } + + var sents []segment.Sentence + cursor := 0 + for _, piece := range ret.Sents { + piece = strings.TrimSpace(piece) + if piece == "" || cursor > len(text) { + continue + } + i := strings.Index(text[cursor:], piece) + if i < 0 { + // A remote segmenter can rewrite text (normalize whitespace, + // say) so that a piece is no longer a literal substring; there + // is no honest span to report for it, so it is dropped rather + // than guessed at. + continue + } + start := cursor + i + cursor = start + len(piece) + sents = append(sents, segment.Sentence{Text: piece, Start: start}) + } + return sents, nil +} + // TextToTokens converts a string to a slice of tagged tokens. // // Tokens from the built-in tagger carry their byte offset within text, so // text[tok.Start:tok.Start+len(tok.Text)] == tok.Text. Tokens from a remote // NLP endpoint do not: that API returns text and tags only, so Start is zero // throughout and callers needing positions must locate the tokens themselves. -func TextToTokens(text string, nlp *Info) []tag.Token { +// +// A remote endpoint's request can fail -- a network error, a timeout, a +// non-2xx status (see post, in http.go) -- so this reports that as a real +// error rather than panicking: there is no recover() anywhere in Vale, so a +// panic here would crash the whole run instead of surfacing as a normal, +// reportable error. TextToContext (internal/core/util.go) and its own +// caller, the `tag` CLI command (cmd/vale/command.go's runTag), already +// return errors the same way; both are updated to thread this one through +// rather than let it panic. +func TextToTokens(text string, nlp *Info) ([]tag.Token, error) { // Determine if (and how) we need to do POS tagging. if nlp == nil || nlp.Endpoint == "" { // Fall back to our internal library (English-only). - return tagText(text) + return tagText(text), nil } result, err := pos(text, nlp.Lang, nlp.Endpoint) if err != nil { - panic(err) + return nil, err } - return result.Tokens + return result.Tokens, nil } // textToTokensWith converts text to tagged tokens with the named tagger. @@ -210,7 +269,45 @@ func textToTokensWith(model, text string, info *Info) ([]tag.Token, error) { // has ever seen, and one shared between documents would need locking on a path // that is otherwise free of it. type TokenCache struct { - tagged map[string][]tag.Token + tagged map[string][]tag.Token + sentences map[string][]segment.Sentence +} + +// Sentences returns the sentence spans of text -- each one's byte offset +// included, not just its bytes -- segmenting it only the first time. +// +// Segmentation does not depend on which tagger a rule names, unlike +// TokensWith, so there is only one cache for it rather than one per model: +// two rules asking for the same block's sentences, whatever tagger either of +// them uses, get the same segmentation pass. +// +// info drives the same local-vs-remote dispatch Info.Compute already makes +// (see SegmentWith): a file configured with a non-English remote endpoint +// gets that endpoint's own sentence boundaries here too, not always local +// Punkt's. +// +// A non-nil error means segmentation itself failed (a remote endpoint's +// request errored); the caller reports that rather than proceeding with a +// partial or stale result. +func (c *TokenCache) Sentences(text string, info *Info) ([]segment.Sentence, error) { + if c == nil { + return SegmentWith(text, info) + } + + if sents, ok := c.sentences[text]; ok { + return sents, nil + } + + sents, err := SegmentWith(text, info) + if err != nil { + return nil, err + } + if c.sentences == nil { + c.sentences = map[string][]segment.Sentence{} + } + c.sentences[text] = sents + + return sents, nil } // Tokens returns the tagged tokens of text, tagging it only the first time. diff --git a/internal/nlp/provider.go b/internal/nlp/provider.go index ed60f5f9..dd3873e5 100644 --- a/internal/nlp/provider.go +++ b/internal/nlp/provider.go @@ -111,6 +111,20 @@ func (b Block) at(offset int) Block { return b } +// IsSentence reports whether b is one sentence a segmenter already produced, +// rather than a block that merely happens to hold exactly one. +// +// doNLP's segmentation loop is the only place that builds a `sentence.`- +// prefixed scope, one per piece of seg(text), so this is a fact about how b +// was constructed, not an inference about what any rule declared. Scope.Matches +// reads the same prefix for its own, unrelated reason (a sentence fragment +// must still satisfy a selector that doesn't ask for one); both go through +// this one definition so the two readings of "is this a sentence block" +// cannot drift apart. +func (b Block) IsSentence() bool { + return strings.HasPrefix(b.Scope, "sentence.") +} + // resolveOffset returns where Text sits within Context. // // Blocks built from markup are handed a context they were carved out of but @@ -175,18 +189,57 @@ type Info struct { // must not reach them. See #1132. func (n *Info) Compute(block *Block, split bool) ([]Block, error) { seg := SentenceTokenizer.Segment - if n.Endpoint != "" && n.Lang != "en" { + + // A remote endpoint's segmentation request can fail -- a network error, + // a timeout, a non-2xx status (see post, in http.go) -- and Compute runs + // during block construction, ahead of every rule's own Run: a plain + // (sentence-scoped) `sequence` rule reaches this exact path just by + // being dispatched at all, not only a `max`/`min` one. There is no + // recover() anywhere in Vale, so panicking here would crash the whole + // run instead of surfacing as this one file's lint error the way + // lintProse already reports any other error Compute returns (wrapped in + // core.NewE100; see internal/lint/lint.go). seg itself has to keep + // returning []string -- it is also the plain, error-free local + // segmenter -- so a remote failure is captured here and turned into + // Compute's own returned error once doNLP is done calling it, rather + // than changing seg's signature for this one caller. + var segErr error + if usesRemoteSegmentation(n) { // We only use external segmentation for non-English text since prose // (our native library) is more efficient. seg = func(text string) []string { ret, err := doSegment(text, n.Lang, n.Endpoint) if err != nil { - panic(err) + segErr = err + return nil } return ret.Sents } } - return n.doNLP(block, seg, split) + + blks := n.doNLP(block, seg, split) + if segErr != nil { + // The request failed partway through block construction: whatever + // doNLP built around the failed call is incomplete, not merely + // missing a few sentences, so it is discarded rather than returned + // alongside the error. + return nil, segErr + } + + return blks, nil +} + +// usesRemoteSegmentation reports whether n should segment sentences via a +// configured remote endpoint's own `/segment` response rather than local +// Punkt: only for non-English text, since prose (Vale's native library) is +// more efficient for English. +// +// Both structural paragraph splitting (Compute, above) and a rule's own +// sentence lookup (File.Sentences, by way of SegmentWith in prose.go) have to +// make this same choice, so it lives in one place rather than two copies that +// could drift apart. +func usesRemoteSegmentation(n *Info) bool { + return n != nil && n.Endpoint != "" && n.Lang != "en" } // offsetOf locates piece within blk.Text and returns its offset in blk's @@ -218,7 +271,7 @@ func offsetOf(blk *Block, base int, piece string, cursor *int) (int, int) { return start, base + start } -func (n *Info) doNLP(blk *Block, seg segmenter, split bool) ([]Block, error) { +func (n *Info) doNLP(blk *Block, seg segmenter, split bool) []Block { blks := []Block{} ctx := blk.Context @@ -252,5 +305,5 @@ func (n *Info) doNLP(blk *Block, seg segmenter, split bool) ([]Block, error) { blks = append( blks, NewLinedBlock(ctx, blk.Text, blk.Scope, idx).at(base).withRuns(blk.Runs, 0)) - return blks, nil + return blks } diff --git a/internal/nlp/provider_test.go b/internal/nlp/provider_test.go index 111bcdb2..77ce1a40 100644 --- a/internal/nlp/provider_test.go +++ b/internal/nlp/provider_test.go @@ -1,10 +1,53 @@ package nlp import ( + "net/http" + "net/http/httptest" "strings" "testing" ) +// IsSentence has to agree exactly with what doNLP's segmentation loop +// builds: true for every block that loop emits, false for the paragraph +// copy and the whole-block copy alongside it. check.Scope.Matches and +// check.Sequence.Run both read this one definition to decide whether a +// block is safe to skip re-segmenting; a false positive here would make +// Run trust an unsegmented block as if it were one sentence. +func TestIsSentence(t *testing.T) { + info := Info{Lang: "en", Segmentation: true, Splitting: true} + + check := func(t *testing.T, blks []Block) { + t.Helper() + for _, b := range blks { + want := strings.HasPrefix(b.Scope, "sentence.") + if got := b.IsSentence(); got != want { + t.Errorf("block %q (scope %q).IsSentence() = %v, want %v", + b.Text, b.Scope, got, want) + } + } + } + + t.Run("split=true: paragraph and whole-block copies are not sentences", func(t *testing.T) { + blk := NewLinedBlock( + "", "One sentence here. Two sentences here.", "text.md", 1) + blks, err := info.Compute(&blk, true) + if err != nil { + t.Fatal(err) + } + check(t, blks) + }) + + t.Run("split=false: whole-block copy is not a sentence", func(t *testing.T) { + blk := NewLinedBlock( + "", "A heading with a sentence.", "text.heading.h2.md", 1) + blks, err := info.Compute(&blk, false) + if err != nil { + t.Fatal(err) + } + check(t, blks) + }) +} + // Compute wraps a block's paragraphs as `paragraph.` only when told the // block holds paragraphs. A heading or a table cell is segmented like any // other prose, but a rule scoped to `paragraph` must not reach it. See #1132. @@ -80,6 +123,55 @@ func TestComputeSplit(t *testing.T) { }) } +// Compute used to panic when a configured remote endpoint's /segment request +// failed -- a network error, a non-2xx status, a malformed response -- which +// crashed the whole vale process rather than reporting a normal lint error. +// Compute runs during block construction, ahead of any rule's own Run, so any +// sentence-scoped rule reaches this exact path just from being dispatched at +// all against a non-English document under a remote endpoint -- there is no +// rule-level error handling downstream to catch a panic here. +// +// A mocked /segment endpoint returning a non-2xx status confirms Compute now +// returns a normal error instead of panicking. Its caller, lintProse +// (internal/lint/lint.go), already wraps any error Compute returns as +// core.NewE100("NLP.Compute", err) -- that handling was already in place, +// only ever unreachable because Compute could not previously return an error +// on this path. +func TestComputeReturnsErrorOnSegmentEndpointFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Sents":[]}`)) + })) + defer server.Close() + + // A non-English language, so Compute reaches the remote branch rather + // than local Punkt. + info := Info{ + Segmentation: true, + Lang: "id", + Endpoint: server.URL, + } + blk := NewLinedBlock("", "I bought a widget. Arrived promptly.", "text.md", 1) + + var blks []Block + var err error + func() { + defer func() { + if p := recover(); p != nil { + t.Fatalf("Compute panicked instead of returning an error: %v", p) + } + }() + blks, err = info.Compute(&blk, false) + }() + + if err == nil { + t.Fatalf("Compute returned a nil error for a failed /segment request, want a non-nil error") + } + if blks != nil { + t.Errorf("blocks = %v, want nil alongside the error", blks) + } +} + // A block that inline markup has rewritten is nowhere in its context, so a // match inside it can only be placed through the runs recorded as it was read. // See #502. diff --git a/testdata/e2e/checks.yaml b/testdata/e2e/checks.yaml index 7f225900..8adebe1f 100644 --- a/testdata/e2e/checks.yaml +++ b/testdata/e2e/checks.yaml @@ -302,6 +302,42 @@ cases: want: | test.md:1:30:T.Ambiguous:Avoid ambiguous pronouns. + - name: sequence/max + about: "#1161 -- `max` counts repeated occurrences of a *tagged* pattern + within a scope, the same way `min` already does above. A `sequence` + rule's declared scope used to always collapse to one sentence at a + time, so a paragraph with two real matches split across two sentences + never tripped a density alert; each sentence individually only ever + had one match." + files: + .vale.ini: | + StylesPath = styles + MinAlertLevel = suggestion + + [*.md] + T.VerbTricolon = YES + styles/T/VerbTricolon.yml: | + extends: sequence + message: "More than one verb tricolon in this paragraph (%d found)." + level: warning + scope: paragraph + max: 1 + tokens: + - tag: VB.* + - pattern: "," + - tag: VB.* + - pattern: "," + - pattern: and + - tag: VB.* + test.md: | + We planned, built, and shipped it. Later we tested, reviewed, and deployed it. + + We planned, built, and shipped it in one clean pass. + args: test.md + exit: 0 + want: | + test.md:1:4:T.VerbTricolon:More than one verb tricolon in this paragraph (2 found). + - name: sequence dir: Sequence args: . @@ -338,3 +374,31 @@ cases: test.txt:21:5:LanguageTool.APOS_ARE:Did you mean "endpoints" instead of "endpoint's"? test.txt:25:1:LanguageTool.Metadata:Use data and metadata as plural nouns. test.txt:29:1:LanguageTool.Metadata:Use data and metadata as plural nouns. + + - name: remote-segment-endpoint-failure-reports-cleanly + about: "a failed /segment request during block construction (Info.Compute, + which runs ahead of every rule -- not just sequence's own -- ahead of + any rule) used to panic instead of surfacing as a normal lint error. + The endpoint is a closed local port, so the failure (connection + refused) is deterministic and needs no network or mock server." + files: + .vale.ini: | + StylesPath = styles + MinAlertLevel = suggestion + NLPEndpoint = http://127.0.0.1:1/ + + [*.md] + Lang = id + T.Seq = YES + styles/T/Seq.yml: | + extends: sequence + message: "matched" + level: error + tokens: + - pattern: foo + test.md: | + This is a test sentence. Here is another one. + args: test.md + exit: 2 + contains: | + E100 [NLP.Compute] Runtime error diff --git a/testdata/e2e/cli.yaml b/testdata/e2e/cli.yaml index 1f63d390..f7f7253c 100644 --- a/testdata/e2e/cli.yaml +++ b/testdata/e2e/cli.yaml @@ -135,3 +135,19 @@ cases: ], "error": "" } + + - name: tag-reports-endpoint-failure + about: "a failed /tag request used to panic (a crash with a goroutine stack + trace) instead of the command reporting a normal error; confirms it no + longer does. The endpoint is a closed local port, so the failure + (connection refused) is deterministic and needs no network or mock + server -- only the panic-vs-clean-error distinction is under test, + not any particular endpoint response." + files: + tagtest.txt: | + A short sentence to tag. + args: tag tagtest.txt id http://127.0.0.1:1/ + exit: 2 + absent: + - "panic:" + - goroutine