diff --git a/internal/llm/client.go b/internal/llm/client.go index c5a60c5..4d77ae6 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -9,6 +9,7 @@ import ( "context" "encoding/json" "fmt" + "log" "net/http" "sync" "time" @@ -153,6 +154,11 @@ type body struct { type resp struct { Choices []struct { Message msg `json:"message"` + // FinishReason — "length" means the token cap cut the generation off. + // Worth a log line on every path (Vikunja #531): a grammar-constrained + // generation that runs to the cap can still parse, so nothing above + // this struct can tell a truncated answer from a whole one. + FinishReason string `json:"finish_reason"` } `json:"choices"` } @@ -205,6 +211,9 @@ func (c *Client) Complete(ctx context.Context, r Req) (string, error) { if len(out.Choices) == 0 { return "", fmt.Errorf("llm: no choices") } + if out.Choices[0].FinishReason == "length" { + log.Printf("llm: generation hit the %d-token cap (finish_reason=length) — output truncated, or the model was looping", r.MaxTokens) + } content := out.Choices[0].Message.Content if content == "" { content = out.Choices[0].Message.ReasoningContent diff --git a/internal/phraser/grammar_bounds_test.go b/internal/phraser/grammar_bounds_test.go new file mode 100644 index 0000000..1fd9d12 --- /dev/null +++ b/internal/phraser/grammar_bounds_test.go @@ -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) + } + } +} diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index ddbaa64..68e1495 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -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": "", "mood": ""}. // @@ -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 diff --git a/internal/router/grammar_bounds_test.go b/internal/router/grammar_bounds_test.go new file mode 100644 index 0000000..f45ebb1 --- /dev/null +++ b/internal/router/grammar_bounds_test.go @@ -0,0 +1,35 @@ +package router + +import ( + "strings" + "testing" +) + +// routeGrammar carried the same unbounded `ws ::= [ \t\n]*` the phrasing +// grammar did (Vikunja #531). It never ran away in practice because this path +// sends routeRepeatPenalty and the phrasing path sent nothing — an accident, +// not a defence. The bound is the defence. +// +// Unlike responseGrammar this grammar does have one legitimate unbounded +// repetition: `("," ws action)*` in root, because a compound utterance is any +// number of actions and capping it would silently drop the last ask. That one +// is bounded in practice by MaxTokens and by each action being fixed-shape. +// Whitespace has no such excuse. +func TestRouteGrammarWhitespaceIsBounded(t *testing.T) { + if !strings.Contains(routeGrammar, `ws ::= [ \t\n]{0,4}`) { + t.Errorf("the ws rule is not bounded — an unbounded one lets the model emit whitespace to the token cap:\n%s", routeGrammar) + } + if strings.Contains(routeGrammar, `[ \t\n]*`) { + t.Error("routeGrammar still contains an unbounded whitespace repetition") + } +} + +// The penalty stays. It curbs the in-field repetition loop the bound cannot +// reach — a model repeating whole words inside a string rule is still inside +// the grammar — and removing it because the grammar is now bounded would be +// reading this fix as broader than it is. +func TestRouterStillSendsARepeatPenalty(t *testing.T) { + if routeRepeatPenalty <= 1.0 { + t.Errorf("routeRepeatPenalty is %v, which is no penalty at all", routeRepeatPenalty) + } +} diff --git a/internal/router/llmrouter.go b/internal/router/llmrouter.go index 341193b..9be0906 100644 --- a/internal/router/llmrouter.go +++ b/internal/router/llmrouter.go @@ -26,6 +26,14 @@ func NewLLMRouter(c Completer) *LLMRouter { return &LLMRouter{c: c} } // prevent free-form drift from a sub-1B model. The string rule is length-bounded // so a repetition loop cannot fill the whole token budget with one field and // truncate the JSON. +// +// EVERY repetition in this grammar is bounded, and ws is the one that matters +// most. An unbounded `ws ::= [ \t\n]*` is a licence to emit whitespace until +// max_tokens: the model opens the JSON, satisfies ws forever, and the only +// thing that stops it is the cap. That cost 24-30s a turn on the phrasing side +// (Vikunja #531), where nothing sent a repeat penalty. This path sends +// routeRepeatPenalty, which masked it here — the bound is what actually +// prevents it, so it does not depend on a sampler setting staying put. const routeGrammar = ` root ::= "[" ws action ("," ws action)* ws "]" action ::= "{" ws "\"intent\"" ws ":" ws intent ("," ws field)* ws "}" @@ -33,7 +41,7 @@ intent ::= "\"fact\"" | "\"reminder\"" | "\"note\"" | "\"query\"" | "\"act\"" | field ::= key ws ":" ws string key ::= "\"key\"" | "\"value\"" | "\"text\"" | "\"verb\"" string ::= "\"" ([^"\\] | "\\" .){0,120} "\"" -ws ::= [ \t\n]* +ws ::= [ \t\n]{0,4} ` // routeSystem — the router prompt. Changed 31-07-2026: the query test now sits