grammar: bound ws, so phrasing stops when it is done (V-531)
A spoken turn took 25-34 seconds and effectively all of it was one phrasing
call generating whitespace. Both interactive turns measured on 2026-08-04
decoded exactly 512 tokens, which is the phrasing MaxTokens, and both ran to
the cap. Background phrasing on the same server in the same window stopped at
32-36 tokens in 4.3s, so it was never the server and never contention.
`ws ::= [ \t\n]*` is a licence to emit whitespace until max_tokens. The model
opens the object, satisfies ws forever, and only the cap stops it. Bounding
the rule fixes it outright with no repeat penalty at all: three runs, three
clean stops at 33 tokens. routeGrammar carried the same rule and is bounded
too — it never ran away only because that path sends routeRepeatPenalty, which
is an accident rather than a defence.
chatReq had no repeat-penalty field at all, so every caller through
chatWithSystem ran at the server default of 1.0 while Replier.PhraseReply sent
1.3 through internal/llm and was protected by accident. Adding it is defence
in depth, not the fix. Two wire structs disagreeing about the sampler is not a
decision anybody made.
finish_reason is parsed on both transports now and a cap hit logs. Both replies
that ran away happened to parse — the grammar had already closed the JSON — so
a truncated generation was indistinguishable from a whole one at every layer
above the response struct.
The phraser test rejects unbounded repetition anywhere in responseGrammar
rather than checking ws by name. A grammar is a budget: every repetition in it
is something the model may do until the token cap, and the cap is not a design.
routeGrammar keeps one, `("," ws action)*`, because a compound utterance is any
number of actions and capping it would drop the last ask.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011x5DgnExQ5XZy8TZPs5bot
This commit is contained in:
@@ -0,0 +1,81 @@
|
||||
package phraser
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"regexp"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// unboundedRepeat finds a `*` or `+` applied to a character class or a group.
|
||||
//
|
||||
// Those are the two forms that let the model generate without limit. A literal
|
||||
// `*` inside a quoted terminal is not one, which is why the pattern anchors on
|
||||
// `]` and `)`.
|
||||
var unboundedRepeat = regexp.MustCompile(`[\]\)]\s*[*+]`)
|
||||
|
||||
// An unbounded ws rule cost 24-30 seconds a turn (Vikunja #531). The model
|
||||
// opened the JSON object, satisfied `ws ::= [ \t\n]*` with whitespace, and ran
|
||||
// to the 512-token cap doing it — both interactive turns measured on
|
||||
// 2026-08-04 decoded exactly 512 tokens of mostly whitespace.
|
||||
//
|
||||
// The rule this test enforces is stronger than "ws is bounded", on purpose. A
|
||||
// grammar is a budget: every repetition in it is something the model is allowed
|
||||
// to do until the token cap, and the cap is not a design. A new rule with a
|
||||
// bare `*` is the same defect wearing a different name, and it would cost
|
||||
// another QA sitting to find.
|
||||
func TestResponseGrammarHasNoUnboundedRepetition(t *testing.T) {
|
||||
for i, line := range strings.Split(responseGrammar, "\n") {
|
||||
if m := unboundedRepeat.FindString(line); m != "" {
|
||||
t.Errorf("responseGrammar line %d has unbounded repetition %q — bound it, a cap is not a design:\n\t%s",
|
||||
i+1, m, strings.TrimSpace(line))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The measured bound. 400 was too tight for the string rule and the comment on
|
||||
// responseGrammar records why; {0,4} for whitespace was measured the same way,
|
||||
// three runs stopping cleanly at 33 tokens with no repeat penalty at all.
|
||||
func TestResponseGrammarWhitespaceIsBounded(t *testing.T) {
|
||||
if !strings.Contains(responseGrammar, `ws ::= [ \t\n]{0,4}`) {
|
||||
t.Errorf("the ws rule is not the measured {0,4} bound:\n%s", responseGrammar)
|
||||
}
|
||||
}
|
||||
|
||||
// The phrasing request must carry a repeat penalty. It is not what fixes #531 —
|
||||
// the bounded grammar is — but chatReq having no such field while
|
||||
// Replier.PhraseReply sent 1.3 is how one phrasing path ran away and the other
|
||||
// did not. Two wire structs disagreeing about the sampler is not a decision
|
||||
// anybody made.
|
||||
func TestPhrasingRequestCarriesARepeatPenalty(t *testing.T) {
|
||||
if phraseRepeatPenalty <= 1.0 {
|
||||
t.Fatalf("phraseRepeatPenalty is %v, which is the server default and no penalty at all", phraseRepeatPenalty)
|
||||
}
|
||||
var got []float64
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
var req chatReq
|
||||
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
|
||||
t.Errorf("decode request: %v", err)
|
||||
}
|
||||
got = append(got, req.RepeatPenalty)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Write([]byte(`{"choices":[{"message":{"content":"{\"response\": \"ага\", \"mood\": \"neutral\"}"},"finish_reason":"stop"}]}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
// Every path, not just one: the defect was a whole transport missing the
|
||||
// field, so a test that checked a single caller would have passed before
|
||||
// the fix as easily as after it.
|
||||
callAllPhrasingPaths(t, NewLLMPhraserAt(srv.URL, Config{LLMNudges: true}))
|
||||
|
||||
if len(got) == 0 {
|
||||
t.Fatal("no request captured")
|
||||
}
|
||||
for i, p := range got {
|
||||
if p != phraseRepeatPenalty {
|
||||
t.Errorf("request %d sent repeat_penalty %v, want %v", i, p, phraseRepeatPenalty)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -593,10 +593,11 @@ func (p *LLMPhraser) chatWithMessages(ctx context.Context, msgs []chatMsg, maxTo
|
||||
}
|
||||
defer release()
|
||||
req := chatReq{
|
||||
Messages: msgs,
|
||||
Temperature: 0.7,
|
||||
MaxTokens: maxTokens,
|
||||
Grammar: p.grammar(),
|
||||
Messages: msgs,
|
||||
Temperature: 0.7,
|
||||
MaxTokens: maxTokens,
|
||||
Grammar: p.grammar(),
|
||||
RepeatPenalty: phraseRepeatPenalty,
|
||||
}
|
||||
body, err := json.Marshal(req)
|
||||
if err != nil {
|
||||
@@ -626,6 +627,7 @@ func (p *LLMPhraser) chatWithMessages(ctx context.Context, msgs []chatMsg, maxTo
|
||||
if len(cr.Choices) == 0 {
|
||||
return "", fmt.Errorf("llm: no choices in response")
|
||||
}
|
||||
logIfTruncated("chat", cr.Choices[0].FinishReason, maxTokens)
|
||||
content := cr.Choices[0].Message.Content
|
||||
if content == "" {
|
||||
content = cr.Choices[0].Message.ReasoningContent
|
||||
@@ -690,8 +692,22 @@ type chatReq struct {
|
||||
// Grammar is llama-server's `grammar` field (GBNF). Same wiring as
|
||||
// internal/llm.Req.Grammar. Empty ⇒ unconstrained sampling.
|
||||
Grammar string `json:"grammar,omitempty"`
|
||||
// RepeatPenalty — defence in depth behind the bounded grammar, not the fix
|
||||
// for #531. This struct had no such field, so every caller through
|
||||
// chatWithSystem ran at the server default of 1.0 while Replier.PhraseReply
|
||||
// sent 1.3 through internal/llm and was protected by accident. Two wire
|
||||
// structs that disagree about the sampler is the condition that let one
|
||||
// path run away and the other not, and it should not survive as a
|
||||
// difference nobody chose.
|
||||
RepeatPenalty float64 `json:"repeat_penalty,omitempty"`
|
||||
}
|
||||
|
||||
// phraseRepeatPenalty — matches Replier.PhraseReply, which has sent 1.3 since
|
||||
// it was written. The value is not tuned here and is not what stops the
|
||||
// whitespace loop; the bounded ws rule is. It is here so the two phrasing
|
||||
// paths sample alike.
|
||||
const phraseRepeatPenalty = 1.3
|
||||
|
||||
// responseGrammar — GBNF constraining the model to the documented phrasing
|
||||
// contract and nothing else: {"response": "<text>", "mood": "<enum>"}.
|
||||
//
|
||||
@@ -710,11 +726,19 @@ type chatReq struct {
|
||||
// long, cut mid-word ("Нужно записать и,"), at every token cap from 256 to 2048.
|
||||
// So the token cap was never what stopped it — this rule was. 1000 characters is
|
||||
// roughly six Russian sentences, still short enough to stop a repetition loop.
|
||||
//
|
||||
// ws is bounded for the same reason and it is the more expensive of the two.
|
||||
// `*` let the model open the object and then satisfy ws with whitespace until
|
||||
// max_tokens, which is 512 here: both interactive turns measured on 2026-08-04
|
||||
// decoded exactly 512 tokens and spent 24-30 seconds doing it, all of it
|
||||
// whitespace (Vikunja #531). Nothing on this path sends a repeat penalty —
|
||||
// chatReq had no field for one — so the sampler never broke the loop. {0,4}
|
||||
// was measured: three runs, three clean stops at 33 tokens, no penalty needed.
|
||||
const responseGrammar = `
|
||||
root ::= "{" ws "\"response\"" ws ":" ws string ws "," ws "\"mood\"" ws ":" ws mood ws "}"
|
||||
mood ::= "\"neutral\"" | "\"happy\"" | "\"thinking\"" | "\"tired\"" | "\"confused\""
|
||||
string ::= "\"" ([^"\\] | "\\" ["\\/bfnrt]){0,1000} "\""
|
||||
ws ::= [ \t\n]*
|
||||
ws ::= [ \t\n]{0,4}
|
||||
`
|
||||
|
||||
// ResponseGrammar exposes responseGrammar to the other callers that emit the
|
||||
@@ -738,9 +762,27 @@ type chatResp struct {
|
||||
Reasoning string `json:"reasoning"`
|
||||
ReasoningContent string `json:"reasoning_content"`
|
||||
} `json:"message"`
|
||||
// FinishReason — "stop" when the model chose to end, "length" when the
|
||||
// token cap cut it off. Parsed since #531, where two turns ran to the
|
||||
// 512-token cap and both happened to parse anyway: the grammar had
|
||||
// already closed the JSON, so a truncated generation was indistinguishable
|
||||
// from a good one at every layer above this struct.
|
||||
FinishReason string `json:"finish_reason"`
|
||||
} `json:"choices"`
|
||||
}
|
||||
|
||||
// logIfTruncated says so when a generation stopped at the token cap.
|
||||
//
|
||||
// A cap hit is never routine. Either the model was looping, which is the #531
|
||||
// shape, or the reply was genuinely longer than maxTokens, which means she cut
|
||||
// herself off mid-sentence. Both are worth a line, and neither produced one
|
||||
// before: the caller sees a parsed string and cannot tell.
|
||||
func logIfTruncated(where, reason string, maxTokens int) {
|
||||
if reason == "length" {
|
||||
log.Printf("phraser: %s hit the %d-token cap (finish_reason=length) — the reply is truncated, or the model was looping", where, maxTokens)
|
||||
}
|
||||
}
|
||||
|
||||
func (p *LLMPhraser) chat(ctx context.Context, userPrompt string) (string, error) {
|
||||
return p.chatWithSystem(ctx, p.systemPrompt(), userPrompt, 512)
|
||||
}
|
||||
@@ -762,9 +804,10 @@ func (p *LLMPhraser) chatWithSystem(ctx context.Context, system, user string, ma
|
||||
{Role: "system", Content: system},
|
||||
{Role: "user", Content: user},
|
||||
},
|
||||
Temperature: 0.7,
|
||||
MaxTokens: maxTokens,
|
||||
Grammar: p.grammar(),
|
||||
Temperature: 0.7,
|
||||
MaxTokens: maxTokens,
|
||||
Grammar: p.grammar(),
|
||||
RepeatPenalty: phraseRepeatPenalty,
|
||||
}
|
||||
body, err := json.Marshal(req)
|
||||
if err != nil {
|
||||
@@ -798,6 +841,7 @@ func (p *LLMPhraser) chatWithSystem(ctx context.Context, system, user string, ma
|
||||
if len(cr.Choices) == 0 {
|
||||
return "", fmt.Errorf("llm: no choices in response")
|
||||
}
|
||||
logIfTruncated("chatWithSystem", cr.Choices[0].FinishReason, maxTokens)
|
||||
content := cr.Choices[0].Message.Content
|
||||
if content == "" {
|
||||
content = cr.Choices[0].Message.ReasoningContent
|
||||
|
||||
Reference in New Issue
Block a user