From d7cdcb63bd734190357f3de5d1e36fbc41edc4ef Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 17:56:40 +0400 Subject: [PATCH 1/2] Stop shipping half-written JSON as a reply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs, one symptom. A run of the talk eval produced replies that were literally "{" and "{\n \"" — those strings went out as things Maven said. First bug: the parser could not tell "the model answered in plain prose" from "the model started a JSON object and got cut off". Both came back as empty, and every caller then shipped the raw text. Now an unfinished object returns an error and each caller uses its own fallback instead. Bare prose with no JSON in it still passes through, because small models do sometimes answer that way and the reply is fine. Second bug, and the actual cause: the grammar capped the response field at 400 characters. I measured it against Qwen3.5-0.8B at three different token caps — 256, 768 and 2048 — and the reply came back exactly 400 characters every time, cut mid-word. So the token limit was never what stopped it. The bound is 1000 now, about six Russian sentences, still low enough to cut off a repetition loop. Token caps go from 256 to 768 on the chat and query paths so 1000 characters of Russian actually fits. The nudge path keeps its own cap; a nudge is meant to be one sentence. Note: cmd/mavend/replier_llm.go has its own copy of this parser with the same bug. Left alone here so this commit stays small — that duplicate is Vikunja #396. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/phraser/broken_json_test.go | 54 ++++++++++++++++++ internal/phraser/grammar_test.go | 5 +- internal/phraser/llmphraser.go | 83 ++++++++++++++++++++++------ 3 files changed, 124 insertions(+), 18 deletions(-) create mode 100644 internal/phraser/broken_json_test.go diff --git a/internal/phraser/broken_json_test.go b/internal/phraser/broken_json_test.go new file mode 100644 index 0000000..c8772b7 --- /dev/null +++ b/internal/phraser/broken_json_test.go @@ -0,0 +1,54 @@ +package phraser + +import ( + "errors" + "strings" + "testing" +) + +// A reply that starts a JSON object and never finishes it is a failed +// generation, not a reply. Before this, the parser returned ("", "") for these +// and every caller then shipped the raw fragment as the thing Maven said. A +// real run produced replies of literally "{" and "{\n \"". +func TestParseResponseMoodRejectsUnfinishedJSON(t *testing.T) { + for _, raw := range []string{ + `{`, + "{\n \"", + `{"response": "неполн`, + `{"response": "текст", "mood":`, + } { + text, mood, err := parseResponseMood(raw) + if !errors.Is(err, errBrokenJSON) { + t.Errorf("parseResponseMood(%q) err = %v, want errBrokenJSON", raw, err) + } + if text != "" || mood != "" { + t.Errorf("parseResponseMood(%q) leaked %q/%q — a fragment must never come back as a reply", raw, text, mood) + } + } +} + +// Bare prose is still fine. Small models sometimes answer without any JSON at +// all, and that reply is usable — so the new error must not swallow it. +func TestParseResponseMoodAllowsBareProse(t *testing.T) { + for _, raw := range []string{ + "норм, а ты как?", + "вот что я нашла: ключ у соседа", + } { + text, mood, err := parseResponseMood(raw) + if err != nil { + t.Errorf("parseResponseMood(%q) err = %v, want nil", raw, err) + } + // No JSON means no fields; the caller ships raw as-is. + if text != "" || mood != "" { + t.Errorf("parseResponseMood(%q) = %q/%q, want empty", raw, text, mood) + } + } +} + +// The measured failure: the model wants more than 400 characters and the old +// grammar cut it off mid-word. Guards the bound against being tightened back. +func TestGrammarStringBoundHasRoomForARealAnswer(t *testing.T) { + if !strings.Contains(responseGrammar, "{0,1000}") { + t.Error("grammar string bound is not 1000; 400 truncated real replies mid-word (see the comment on responseGrammar)") + } +} diff --git a/internal/phraser/grammar_test.go b/internal/phraser/grammar_test.go index fe262a6..bf6e713 100644 --- a/internal/phraser/grammar_test.go +++ b/internal/phraser/grammar_test.go @@ -97,7 +97,10 @@ func TestGrammarStringRuleIsNotASCIIOnly(t *testing.T) { // Russian body with an escaped quote inside, hand-built to test the contract. func TestGrammarShapedJSONParses(t *testing.T) { raw := `{"response": "он сказал \"привет\" и ушёл.\nвот так.", "mood": "confused"}` - text, mood := parseResponseMood(raw) + text, mood, err := parseResponseMood(raw) + if err != nil { + t.Fatalf("grammar-shaped JSON did not parse: %v", err) + } if want := "он сказал \"привет\" и ушёл.\nвот так."; text != want { t.Errorf("response = %q, want %q", text, want) } diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index d6ad483..3fc90c3 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -190,7 +190,12 @@ func (p *LLMPhraser) PhraseNudge(ctx context.Context, c loop.Candidate) (deliver if err != nil { return delivery.PhrasedNudge{}, err } - body, mood := parseResponseMood(resp) + body, mood, perr := parseResponseMood(resp) + if perr != nil { + // Truncated JSON. Not a nudge — use the plain Russian fallback. + log.Printf("phraser: PhraseNudge: %v", perr) + body, mood = "", "" + } if body == "" { // fallback: try old body/summary format body, _ = parsePhrase(resp) @@ -216,11 +221,16 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] // prompt is the single tested source in router.KnowledgePrompt. sys := persona.Prepend(p.cfg.ContextBlock, router.KnowledgePrompt()) prompt := fmt.Sprintf("Пользователь спрашивает: \"%s\".", utterance) - resp, err := p.chatWithSystem(ctx, sys, prompt, 256) + resp, err := p.chatWithSystem(ctx, sys, prompt, 768) if err != nil || resp == "" { return "не знаю.", nil } - if text, _ := parseResponseMood(resp); text != "" { + text, _, perr := parseResponseMood(resp) + if perr != nil { + log.Printf("phraser: PhraseQuery: %v", perr) + return "не знаю.", nil + } + if text != "" { return text, nil } return resp, nil @@ -233,14 +243,19 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] `Он спрашивает: "%s". В твоих заметках по этому вопросу написано: "%s". Ответь ему коротко и своими словами. Если в заметках ответа нет — так и скажи.`, utterance, strings.Join(notes, `"; "`), ) - resp, err := p.chatWithSystem(ctx, sys, prompt, 256) - if err != nil { + resp, err := p.chatWithSystem(ctx, sys, prompt, 768) + text, _, perr := parseResponseMood(resp) + if err != nil || perr != nil { + // Read the notes out rather than ship a broken fragment. + if perr != nil { + log.Printf("phraser: PhraseQuery: %v", perr) + } if len(notes) == 1 { return "вот что я нашла: " + notes[0], nil } return "вот что я нашла: " + strings.Join(notes, "; "), nil } - if text, _ := parseResponseMood(resp); text != "" { + if text != "" { return text, nil } return resp, nil @@ -263,12 +278,17 @@ func (p *LLMPhraser) PhraseChat(ctx context.Context, utterance string, history [ combined += utterance msgs = append(msgs, chatMsg{Role: "user", Content: strings.TrimSpace(combined)}) - resp, err := p.chatWithMessages(ctx, msgs, 512) + resp, err := p.chatWithMessages(ctx, msgs, 768) if err != nil { log.Printf("phraser: PhraseChat: %v", err) return "поговорили.", nil } - if text, _ := parseResponseMood(resp); text != "" { + text, _, perr := parseResponseMood(resp) + if perr != nil { + log.Printf("phraser: PhraseChat: %v", perr) + return "поговорили.", nil + } + if text != "" { return text, nil } // fallback: plain text without JSON @@ -351,7 +371,12 @@ func (p *LLMPhraser) PhraseReminder(ctx context.Context, d loop.ReminderDecision if err != nil { return delivery.PhrasedReminder{}, err } - body, mood := parseResponseMood(resp) + body, mood, perr := parseResponseMood(resp) + if perr != nil { + // Truncated JSON. Fall through to the reminder's own text. + log.Printf("phraser: PhraseReminder: %v", perr) + body, mood = "", "" + } if body == "" { // fallback: try old body/summary format body, _ = parsePhrase(resp) @@ -396,10 +421,16 @@ type chatReq struct { // Russian, so an ASCII-only rule would make every reply empty. The escape rule // is what lets the model close a string it opened with a quote inside. Length // is bounded so a repetition loop truncates the field, not the JSON object. +// +// That bound was 400 and 400 was too tight. Measured against Qwen3.5-0.8B: on +// "почему гром слышно позже молнии?" the reply came back exactly 400 characters +// 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. const responseGrammar = ` root ::= "{" ws "\"response\"" ws ":" ws string ws "," ws "\"mood\"" ws ":" ws mood ws "}" mood ::= "\"neutral\"" | "\"happy\"" | "\"thinking\"" | "\"tired\"" | "\"confused\"" -string ::= "\"" ([^"\\] | "\\" ["\\/bfnrt]){0,400} "\"" +string ::= "\"" ([^"\\] | "\\" ["\\/bfnrt]){0,1000} "\"" ws ::= [ \t\n]* ` @@ -633,21 +664,39 @@ type responseMood struct { Mood string `json:"mood"` } +// errBrokenJSON — the model started a JSON object and never finished it. +// That is a failed generation, not a reply. Callers must use their fallback. +var errBrokenJSON = fmt.Errorf("phraser: model output starts as JSON but does not parse") + // parseResponseMood extracts {"response","mood"} from LLM output, tolerant -// of thinking tokens and extra text before/after the JSON block. Returns -// ("", "") when no valid JSON is found. -func parseResponseMood(raw string) (response, mood string) { +// of thinking tokens and extra text before/after the JSON block. +// +// Three outcomes: +// - parsed fine → the fields, nil error. +// - output never looked like JSON → ("", "", nil). The caller may ship it +// as-is; small models sometimes answer in bare prose and that is fine. +// - output starts with "{" but does not parse → errBrokenJSON. The grammar +// guarantees a valid *prefix*, so a generation that hits the token cap +// mid-object comes back as a fragment like `{` or `{\n "`. Shipping that +// as a reply is the bug this error exists to stop. +func parseResponseMood(raw string) (response, mood string, err error) { cleaned := strings.TrimSpace(raw) start := strings.Index(cleaned, "{") end := strings.LastIndex(cleaned, "}") if start < 0 || end < 0 || end <= start { - return "", "" + if strings.HasPrefix(cleaned, "{") { + return "", "", errBrokenJSON + } + return "", "", nil } var parsed responseMood - if err := json.Unmarshal([]byte(cleaned[start:end+1]), &parsed); err != nil { - return "", "" + if e := json.Unmarshal([]byte(cleaned[start:end+1]), &parsed); e != nil { + if strings.HasPrefix(cleaned, "{") { + return "", "", errBrokenJSON + } + return "", "", nil } - return parsed.Response, parsed.Mood + return parsed.Response, parsed.Mood, nil } func parsePhrase(raw string) (body, summary string) { -- 2.52.0 From aa8f5b2ee2b61049d544145ae287e97cf994865f Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 17:57:39 +0400 Subject: [PATCH 2/2] Make the nonempty check look for actual words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It scored 27/27 on a run where two replies were "{" and "{\n \"". It only tested that the string was not blank, so punctuation counted as content and the worst replies of the run passed the first check. Now a reply needs at least one letter, Cyrillic or Latin. Latin counts because answers about ssd or vpn are legitimately part English. Digits alone fail too. The same run answered "сколько варить яйцо вкрутую?" with "15-16" — no unit, no words, and the wrong number as well. That is not something she said. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/phraser/eval/address_multi_test.go | 32 +++++++++++++++++++++ internal/phraser/eval/checks.go | 15 +++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/internal/phraser/eval/address_multi_test.go b/internal/phraser/eval/address_multi_test.go index 378f8f1..d4bc059 100644 --- a/internal/phraser/eval/address_multi_test.go +++ b/internal/phraser/eval/address_multi_test.go @@ -31,3 +31,35 @@ func TestAddressDeduplicates(t *testing.T) { t.Errorf("detail repeats the same break %d times: %q", n, res.Detail) } } + +// The fragments a real run produced. All of them scored as non-empty replies +// before checkNonEmpty looked for letters. +func TestNonEmptyNeedsLetters(t *testing.T) { + for _, body := range []string{ + "{", + "{\n \"", + "15-16", + `{"`, + " ", + "...", + } { + if got := checkNonEmpty(body); got.Pass { + t.Errorf("checkNonEmpty(%q) passed — that is not a reply", body) + } + } +} + +// And it must not start failing real replies. Latin counts as well as Cyrillic: +// answers about ssd or vpn are legitimately part English. +func TestNonEmptyAcceptsRealReplies(t *testing.T) { + for _, body := range []string{ + "норм, а ты как?", + "вот что я нашла: ключ у соседа", + "ssd быстрее hdd.", + "9 минут.", + } { + if got := checkNonEmpty(body); !got.Pass { + t.Errorf("checkNonEmpty(%q) failed: %s", body, got.Detail) + } + } +} diff --git a/internal/phraser/eval/checks.go b/internal/phraser/eval/checks.go index 39ee366..29a198b 100644 --- a/internal/phraser/eval/checks.go +++ b/internal/phraser/eval/checks.go @@ -623,11 +623,24 @@ const ( CheckEllipsis = "ellipsis" // she finished the sentence ) +// A reply needs words in it, not just characters. This check used to test for a +// non-empty string, which scored 27/27 on a run where two replies were "{" and +// "{\n \"" — punctuation passed as content. Braces, quotes, digits and spaces +// are all empty in the only sense that matters. +// +// Digits alone fail too, and that is deliberate: the same run answered "сколько +// варить яйцо вкрутую?" with "15-16". No unit, no words, and it is also the +// wrong number. Whatever that is, it is not something she said. func checkNonEmpty(body string) Result { if strings.TrimSpace(body) == "" { return Result{CheckNonEmpty, false, "empty reply"} } - return Result{CheckNonEmpty, true, ""} + for _, r := range body { + if unicode.IsLetter(r) { + return Result{CheckNonEmpty, true, ""} + } + } + return Result{CheckNonEmpty, false, fmt.Sprintf("no letters in the reply %q — punctuation or digits only", strings.TrimSpace(body))} } // checkEllipsis — a reply ending in "…" or "..." is a generation that ran out of -- 2.52.0