fix: remote NLP endpoint failures surface as errors instead of panicking - #1167
Closed
theredspoon wants to merge 1 commit into
Closed
fix: remote NLP endpoint failures surface as errors instead of panicking#1167theredspoon wants to merge 1 commit into
theredspoon wants to merge 1 commit into
Conversation
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
…ing each sentence separately NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167, which fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit.
This was referenced Sep 2, 2026
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
…ing each sentence separately NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167, which fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
…ing each sentence separately NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167, which fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit.
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.
theredspoon
force-pushed
the
fix/remote-nlp-endpoint-errors
branch
from
September 2, 2026 03:36
63e807a to
b41c3cc
Compare
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
…ing each sentence separately NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167, which fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit.
theredspoon
added a commit
to theredspoon/vale
that referenced
this pull request
Sep 2, 2026
NewSequence unconditionally narrowed every declared scope to sentence-level, so a rule using max/min could never aggregate matches across a paragraph's sentences. Threshold-opted-in rules now keep their real declared scope; Run tags each sentence of that scope separately instead of tagging the whole block once and inferring sentence boundaries afterward, so a match can never span two sentences by construction. An undeclared scope on a threshold rule now defaults to paragraph plus every other prose-container scope, matching what a plain sequence rule's undeclared scope already reaches, via one shared list in internal/core instead of two independently-maintained copies. Built on vale-cli#1167 (fixes three pre-existing panics on remote NLP endpoint failures this feature's own paths would otherwise have hit) and vale-cli#1169 (fixes a sentenceScope bug that review of this feature found as a real, dispatched double-report).
Member
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
nlp.TextToTokens(reached fromcore.TextToContext, reached from thetagCLI command) andnlp.Info.Compute(which runs during block construction, ahead of every rule's ownRun) both panicked when a configured remote NLP endpoint's request failed: a network error, a non-2xx status, or a malformed response. That crashed the wholevaleprocess instead of returning a normal, reportable error.Separately, the shared HTTP transport (
nlp.post) ignored response status codes entirely, so 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.Fix
postnow rejects any non-2xx response with a real error instead of decoding its body.TextToTokensreturns an error instead of panicking on a failed/tagrequest.TextToContextandrunTag(thetagCLI command) are updated to thread that error through.Info.Computereturns an error instead of panicking on a failed/segmentrequest. Its existing caller,lintProse, already wraps any errorComputereturns ascore.NewE100("NLP.Compute", err), that handling was already in place, only unreachable becauseComputecouldn't previously return an error on this path.Each fix has its own regression test, both for
post's status check directly and for the panic-to-error behavior of each affected function.Testing
Two
internal/e2ecases cover the user-visible behavior change end to end, each verified against the actual pre-fix binary (built at the parent commit) to confirm it currently panics there before confirming it passes cleanly here:checks/remote-segment-endpoint-failure-reports-cleanly: a lint run that hits theInfo.Computepanic path (asequencerule, non-EnglishLang,NLPEndpointpointed at a closed local port for a deterministic, dependency-free connection-refused failure).cli/tag-reports-endpoint-failure: thevale tagCLI command hitting the same kind of failure viaTextToTokens/TextToContext.Full repo suite and
-raceare clean, includinginternal/e2e.Related
Found during review of #1162, where these were originally bundled. Split out here as an independent, single-concern fix per the smaller-PR preference from #938.