Merge task/531-bug-unbounded-ws-in-responsegrammar-lets

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

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)
+		}
+	}
+}

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": "<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

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)
+	}
+}

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