From b41c3cc400ad5f97db24778124e692e127f55bd4 Mon Sep 17 00:00:00 2001 From: Nim G Date: Wed, 2 Sep 2026 00:34:57 -0300 Subject: [PATCH] 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