engine(text-generator): trim stop-string suffix at char boundary, not whole token - #296
engine(text-generator): trim stop-string suffix at char boundary, not whole token#296jamesburton wants to merge 2 commits into
Conversation
… whole token (#107) When a `StopStringCondition` matched, `TextGenerator` removed the entire last token from the generated-id list. That over-trimmed whenever the stop string was a strict suffix of the last token's decoded text — the headline example from the issue: last token decodes to `"ld<|im_end|>"`, stop string is `"<|im_end|>"`, the user saw `"Hello, wor"` instead of `"Hello, world"`. Introduces `StopSuffixTrimmer` — a static helper that finds the longest stop-string suffix of the decoded text and trims it at the character (UTF-16) boundary, defending against splitting a surrogate pair. `CheckStopConditions` now also reports the matched condition index; when the match was a `StopStringCondition`, the call sites keep the last token in `generatedIds` and `BuildResponse` performs the suffix trim on the fully-decoded text. EOS / max-tokens / other non-string stop conditions keep the original token-removal semantics (their "token" is conceptually the terminator itself, not text-bearing). Regression tests cover the headline strict-suffix case, longest-of-multiple matches, the no-match passthrough, ignoring non-`StopStringCondition` entries, and the surrogate-pair safety trim. Note: this PR focuses on the non-streaming `Generate` paths (prefill + greedy/spec decode loops returning an `InferenceResponse`). The streaming `GenerateStream` path keeps the existing token-removal behaviour; applying the same character-level trim to the streaming SSE output is a follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds character-level trimming for stop-string suffix matches so generation preserves token prefixes while still removing matched stop strings from the final decoded text.
Changes:
- Introduces
StopSuffixTrimmerto trim the longest stop-string suffix while preserving valid UTF-16 boundaries. - Updates
TextGeneratorto keep the last token id on stop-string matches and trim the decoded text inBuildResponse. - Adds unit tests covering strict-suffix, longest-match, non-match, non-stop-string conditions, and surrogate-pair boundaries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/DotLLM.Tests.Unit/Engine/Samplers/StopConditions/StopSuffixTrimmerTests.cs | Adds regression/unit coverage for suffix trimming behavior and UTF-16 safety. |
| src/DotLLM.Engine/TextGenerator.cs | Keeps last token id for stop-string stops and trims stop-string suffix in the final decoded text. |
| src/DotLLM.Engine/Samplers/StopConditions/StopSuffixTrimmer.cs | Implements longest stop-string suffix detection and safe character-boundary trimming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| detok.GetTailView(stopTailSize, stopScratch), out int firstMatchedIdx); | ||
| if (stopResult != StopResult.Continue) | ||
| { | ||
| bool isStopStringMatch = IsStopStringMatch(stopConditions, firstMatchedIdx); | ||
| if (stopResult == StopResult.Stop) | ||
| generatedIds.RemoveAt(generatedIds.Count - 1); | ||
| { | ||
| if (!isStopStringMatch) | ||
| generatedIds.RemoveAt(generatedIds.Count - 1); |
| if (stopResult != StopResult.Continue) | ||
| { | ||
| bool isStopStringMatch = IsStopStringMatch(stopConditions, firstMatchedIdx); | ||
| if (stopResult == StopResult.Stop) | ||
| generatedIds.RemoveAt(generatedIds.Count - 1); | ||
| { | ||
| if (!isStopStringMatch) | ||
| generatedIds.RemoveAt(generatedIds.Count - 1); | ||
| // Stop-string match: keep the last token in the id list; BuildResponse | ||
| // will trim the matched stop-string suffix at the character boundary. | ||
| } | ||
| else | ||
| onTokenGenerated?.Invoke(firstTokenId); |
|
|
||
| string trimmed = StopSuffixTrimmer.TrimMatchedSuffix(fullText, conditions); | ||
|
|
||
| Assert.Same(fullText, trimmed); |
…ent (#107) Address Copilot review. The keep-last-token decision was derived from the type of the FIRST stop condition to return Stop, so registration order mattered: an EOS condition ahead of a StopStringCondition matching the same tail would drop the whole last token and resurrect the over-trim bug. Replace the matched-index plumbing with HasStopStringSuffix(decodedTail, conditions), built on StopSuffixTrimmer.MatchedSuffixLength — the same ordinal EndsWith predicate StopStringCondition itself uses, evaluated over all conditions. The out-parameter CheckStopConditions overload and IsStopStringMatch are gone; the tail view is hoisted into a local at each of the three call sites. Adds a discriminating regression test with EOS + MaxTokens registered before the matching stop string. Also documents onTokenGenerated semantics: it is a token-level progress hook, the stop-triggering token is deliberately never passed to it (an Action<int> cannot express "emit part of this token", and passing the id would leak the stop string), so the callback stream may be a strict prefix of InferenceResponse.Text.
|
Thanks — all three comments worked through. Pushed as 4abc7fb. 1. Ordering dependence of
|
Summary
Addresses an item from #107. When a stop string spans the boundary inside a generated token, the text generator now trims at the exact character boundary inside that token rather than discarding the whole token.
Test results
StopSuffixTrimmerTests(140 lines, 3 files changed, 282 insertions).Notes for review
Single-commit correctness fix on
main. This is the precursor to #121's streaming stop-string fix (which stacks on this branch).