From 7d0601cf20d5f665458a892875faab200f496c15 Mon Sep 17 00:00:00 2001 From: Nim G Date: Wed, 2 Sep 2026 00:34:57 -0300 Subject: [PATCH 1/4] fix: remote NLP endpoint failures surface as errors instead of panicking The remote-tagging and remote-segmentation call sites (TextToTokens, Info.Compute) panicked on a failed /tag or /segment request instead of returning an error, crashing the whole vale process rather than surfacing a normal, reportable error. TextToContext and the `tag` CLI command are updated to thread the error through rather than let it panic. The shared HTTP transport also ignored response status codes: a non-2xx response with a technically-valid JSON body (e.g. `500 {"sents":[]}`) was silently decoded as a successful, empty result instead of a failure. Each fix has its own regression test. Two internal/e2e scenarios cover the user-visible behavior end to end, both against a closed local port so the failure (connection refused) is deterministic and needs no network or mock server: a lint run whose Info.Compute hits a failed /segment request during block construction, and the `tag` CLI command's /tag request. Verified against the pre-fix commit that both currently fail this way (a panic with a goroutine stack trace) before this fix, and pass cleanly after it. --- cmd/vale/command.go | 5 +- internal/core/util.go | 11 +++- internal/core/util_test.go | 36 +++++++++++ internal/nlp/http.go | 14 +++++ internal/nlp/http_test.go | 114 ++++++++++++++++++++++++++++++++++ internal/nlp/prose.go | 17 +++-- internal/nlp/provider.go | 32 ++++++++-- internal/nlp/provider_test.go | 51 +++++++++++++++ testdata/e2e/checks.yaml | 28 +++++++++ testdata/e2e/cli.yaml | 16 +++++ 10 files changed, 312 insertions(+), 12 deletions(-) create mode 100644 internal/nlp/http_test.go 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/core/util.go b/internal/core/util.go index 9a5f96b2..9230f4d3 100755 --- a/internal/core/util.go +++ b/internal/core/util.go @@ -265,14 +265,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 +293,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/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..901e79ba 100644 --- a/internal/nlp/prose.go +++ b/internal/nlp/prose.go @@ -172,17 +172,26 @@ func tagTextWith(model, text string) ([]tag.Token, error) { // 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. diff --git a/internal/nlp/provider.go b/internal/nlp/provider.go index ed60f5f9..174c50be 100644 --- a/internal/nlp/provider.go +++ b/internal/nlp/provider.go @@ -175,18 +175,42 @@ type Info struct { // must not reach them. See #1132. func (n *Info) Compute(block *Block, split bool) ([]Block, error) { seg := SentenceTokenizer.Segment + + // 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. 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 n.Endpoint != "" && n.Lang != "en" { // 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 } // offsetOf locates piece within blk.Text and returns its offset in blk's @@ -218,7 +242,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 +276,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..e38899ec 100644 --- a/internal/nlp/provider_test.go +++ b/internal/nlp/provider_test.go @@ -1,6 +1,8 @@ package nlp import ( + "net/http" + "net/http/httptest" "strings" "testing" ) @@ -80,6 +82,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..df11597f 100644 --- a/testdata/e2e/checks.yaml +++ b/testdata/e2e/checks.yaml @@ -338,3 +338,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 From 78c85b765271ca4b66522e906fe0c659c742f566 Mon Sep 17 00:00:00 2001 From: Nim G Date: Tue, 1 Sep 2026 23:52:34 -0300 Subject: [PATCH 2/4] fix: a sequence rule with a negated scope double-reports every match sentenceScope's negation branch was a no-op for a bare negated term: `~list` narrowed to `~list`, itself, via strings.CutPrefix re-adding the same prefix it had just stripped. A negated term never mentions `sentence`, so asksForSentence (scope.go) then skipped every `sentence.*` fragment block for such a rule, and Scope.Matches instead matched both the whole-block copy and its own paragraph wrapper for the same text. One real match dispatched to Run twice, once per block, and produced two identical alerts. The negation branch now AND-s `sentence` in front of the term instead of leaving it untouched, so `~list` narrows to `sentence&~list`, sentences outside a list, the same as every other declared scope already does. --- internal/check/sequence.go | 14 ++++--- internal/check/sequence_test.go | 69 ++++++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/internal/check/sequence.go b/internal/check/sequence.go index 44faa2de..46fef37e 100644 --- a/internal/check/sequence.go +++ b/internal/check/sequence.go @@ -609,11 +609,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 diff --git a/internal/check/sequence_test.go b/internal/check/sequence_test.go index 6bd5ec12..bf2c22cb 100644 --- a/internal/check/sequence_test.go +++ b/internal/check/sequence_test.go @@ -71,7 +71,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 +111,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. From 99f0ca005dc7041e2277eb7f2162c6ce56236bc9 Mon Sep 17 00:00:00 2001 From: Nim G Date: Wed, 2 Sep 2026 00:38:05 -0300 Subject: [PATCH 3/4] feat: add max/min count thresholds to sequence rules NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on #1167 (fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit) and #1169 (fixes a sentenceScope bug that review of this feature found as a real, dispatched double-report). --- internal/check/sequence.go | 338 +++++++-- internal/check/sequence_test.go | 1206 +++++++++++++++++++++++++++++++ internal/core/file.go | 29 + internal/core/util.go | 28 + internal/lint/ast.go | 21 +- internal/nlp/prose.go | 90 ++- internal/nlp/provider.go | 19 +- testdata/e2e/checks.yaml | 36 + 8 files changed, 1693 insertions(+), 74 deletions(-) diff --git a/internal/check/sequence.go b/internal/check/sequence.go index 46fef37e..e6faf26a 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 @@ -638,90 +722,218 @@ 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. + // + // A plain rule's own scope is always narrowed to `sentence` (see + // sentenceScope), so blk.Text there is already a single sentence and + // this segments it right back into exactly one -- a no-op, not a + // behavior change. A `max`/`min` rule's declared scope is not + // narrowed, so this is what lets it see every sentence of a + // paragraph (or whichever block it named) in one Run call. + 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. + 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_test.go b/internal/check/sequence_test.go index bf2c22cb..389e725a 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} } @@ -477,3 +1177,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 9230f4d3..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", } 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/prose.go b/internal/nlp/prose.go index 901e79ba..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,6 +167,55 @@ 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 @@ -219,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 174c50be..7f4e661d 100644 --- a/internal/nlp/provider.go +++ b/internal/nlp/provider.go @@ -178,7 +178,9 @@ func (n *Info) Compute(block *Block, split bool) ([]Block, error) { // 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. There is no + // 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 @@ -188,7 +190,7 @@ func (n *Info) Compute(block *Block, split bool) ([]Block, error) { // 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 n.Endpoint != "" && n.Lang != "en" { + 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 { @@ -213,6 +215,19 @@ func (n *Info) Compute(block *Block, split bool) ([]Block, error) { 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 // context, or -1 if it cannot be placed. // diff --git a/testdata/e2e/checks.yaml b/testdata/e2e/checks.yaml index df11597f..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: . From c058ccfe007711e7fa433e7beb99f906f8dd1540 Mon Sep 17 00:00:00 2001 From: Nim G Date: Wed, 2 Sep 2026 00:15:31 -0300 Subject: [PATCH 4/4] perf: skip redundant re-segmentation for sentence blocks in sequence.Run Run called f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to `sentence`, paying a redundant segmentation pass (a full remote round-trip under a configured non-English endpoint) on text that had already been segmented once, by the same segmenter, to build the block Run was handed. Trusting a rule's declared scope unconditionally to justify skipping that call is unsafe: sentenceScope's negation branch was a real bug (fixed separately in #1169) that could otherwise have made this optimization reintroduce a cross-sentence false positive. Gated instead on a structural fact about the block itself: Block.IsSentence reports whether blk's scope was built by doNLP's segmentation loop, the only place that produces a `sentence.`-prefixed scope. Re-segmenting one segmenter's own output with that same segmenter can only ever reproduce it, so the call is skipped exactly when it is truly redundant. Scope.Matches reads the same helper for its own, unrelated `sentence.` check, so the two readings of "is this a sentence block" cannot drift apart. Covered by a new invariant test over the whole scope grammar, a dispatched regression test proving this doesn't reopen the negated-scope false positive #1169 fixed, an IsSentence contract test, and a call-count test proving the round-trip savings directly against a mocked remote endpoint. --- internal/check/scope.go | 2 +- internal/check/sequence.go | 31 ++-- internal/check/sequence_bench_test.go | 43 ++++++ internal/check/sequence_test.go | 213 ++++++++++++++++++++++++++ internal/nlp/provider.go | 14 ++ internal/nlp/provider_test.go | 41 +++++ 6 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 internal/check/sequence_bench_test.go 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 e6faf26a..d3f5bdfa 100644 --- a/internal/check/sequence.go +++ b/internal/check/sequence.go @@ -747,19 +747,30 @@ func (s Sequence) Run(blk nlp.Block, f *core.File, _ *core.Config) ([]core.Alert // for tagging or by an earlier rule is not segmented again, since // sentences are read from the file's shared cache. // - // A plain rule's own scope is always narrowed to `sentence` (see - // sentenceScope), so blk.Text there is already a single sentence and - // this segments it right back into exactly one -- a no-op, not a - // behavior change. A `max`/`min` rule's declared scope is not - // narrowed, so this is what lets it see every sentence of a - // paragraph (or whichever block it named) in one Run call. - sentences, serr := f.Sentences(blk.Text) - if serr != nil { - return nil, serr + // 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. + // 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}} } 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 389e725a..5d2b256c 100644 --- a/internal/check/sequence_test.go +++ b/internal/check/sequence_test.go @@ -903,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. diff --git a/internal/nlp/provider.go b/internal/nlp/provider.go index 7f4e661d..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 diff --git a/internal/nlp/provider_test.go b/internal/nlp/provider_test.go index e38899ec..77ce1a40 100644 --- a/internal/nlp/provider_test.go +++ b/internal/nlp/provider_test.go @@ -7,6 +7,47 @@ import ( "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.