diff --git a/internal/phraser/broken_json_test.go b/internal/phraser/broken_json_test.go index c8772b7..d766b8a 100644 --- a/internal/phraser/broken_json_test.go +++ b/internal/phraser/broken_json_test.go @@ -52,3 +52,53 @@ func TestGrammarStringBoundHasRoomForARealAnswer(t *testing.T) { t.Error("grammar string bound is not 1000; 400 truncated real replies mid-word (see the comment on responseGrammar)") } } + +// A multi-line reply is the sixty-failure shape from the 2026-08-05 temperature +// sweep (Vikunja #537). The model wrote a literal newline inside the string, +// which the old grammar allowed and json.Unmarshal rejects with "invalid +// character '\n' in string literal". The object starts with "{", so it came back +// as errBrokenJSON and the case answered with nothing at all. +// +// The grammar is the fix. This is the parser's own second line, for the paths +// that send no grammar: the reply is readable, so read it. +func TestParseResponseMoodRepairsARawNewline(t *testing.T) { + raw := "{\"response\": \"первая строка\nвторая строка\", \"mood\": \"neutral\"}" + text, mood, err := parseResponseMood(raw) + if err != nil { + t.Fatalf("err = %v, want nil — a raw newline is repairable, not a failed generation", err) + } + if want := "первая строка\nвторая строка"; text != want { + t.Errorf("response = %q, want %q", text, want) + } + if mood != "neutral" { + t.Errorf("mood = %q, want neutral", mood) + } +} + +// Repairing must not rewrite JSON that was already fine: an escaped newline +// stays one newline, and a backslash the model escaped properly is left alone. +func TestEscapeRawControlsLeavesValidJSONAlone(t *testing.T) { + raw := `{"response": "строка\nдве \\ и \"кавычки\"", "mood": "happy"}` + if got := escapeRawControls(raw); got != raw { + t.Errorf("escapeRawControls rewrote valid JSON:\n got %q\nwant %q", got, raw) + } + text, _, err := parseResponseMood(raw) + if err != nil { + t.Fatalf("err = %v", err) + } + if want := "строка\nдве \\ и \"кавычки\""; text != want { + t.Errorf("response = %q, want %q", text, want) + } +} + +// A tab and a bare control byte take the same path as the newline. +func TestParseResponseMoodRepairsOtherControls(t *testing.T) { + raw := "{\"response\": \"таб\tи \x01байт\", \"mood\": \"tired\"}" + text, _, err := parseResponseMood(raw) + if err != nil { + t.Fatalf("err = %v, want nil", err) + } + if want := "таб\tи \x01байт"; text != want { + t.Errorf("response = %q, want %q", text, want) + } +} diff --git a/internal/phraser/grammar_test.go b/internal/phraser/grammar_test.go index 2117c98..f678923 100644 --- a/internal/phraser/grammar_test.go +++ b/internal/phraser/grammar_test.go @@ -90,8 +90,13 @@ func TestNoGrammarConfigDisablesIt(t *testing.T) { // The grammar's string rule must accept any codepoint, not just ASCII. Replies // are Russian: an ASCII-only class would constrain the model into empty replies. func TestGrammarStringRuleIsNotASCIIOnly(t *testing.T) { - if !strings.Contains(responseGrammar, `([^"\\] | "\\" ["\\/bfnrt])`) { - t.Error("string rule is not the any-codepoint-except-quote-and-backslash class; Cyrillic replies would be impossible") + if !strings.Contains(responseGrammar, `[^"\\\x00-\x1F]`) { + t.Error("string rule is not the any-codepoint-except-quote-backslash-and-controls class; Cyrillic replies would be impossible") + } + // The control range must be out (Vikunja #537): a raw newline inside a JSON + // string is not JSON, and the model wrote one whenever it wanted two lines. + if strings.Contains(responseGrammar, `([^"\\] |`) { + t.Error("string rule still admits raw control characters; a multi-line reply will fail to parse") } } diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index 0d6b6f9..7647567 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -724,10 +724,21 @@ const phraseRepeatPenalty = 1.3 // which eats the token budget before the JSON closes. Modelled on // routeGrammar in internal/router/llmrouter.go so the two read alike. // -// text accepts ANY codepoint except the two JSON must escape — the replies are -// 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. +// text accepts ANY codepoint except the two JSON must escape and the control +// range — the replies are 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. +// +// The control range is excluded because a raw newline inside a JSON string is +// not JSON (Vikunja #537). The class used to be `[^"\\]`, which let the model +// write a multi-line reply that satisfied the grammar and then failed +// json.Unmarshal with "invalid character '\n' in string literal" — the object +// starts with "{", so it came back as errBrokenJSON and the case answered with +// an empty string. Sixty of the failures in the 2026-08-05 temperature sweep +// were that one error, and it never once hit the token cap, which is why the +// truncation reading was wrong. The escape alternatives are llama.cpp's own +// json.gbnf: a model that wants a line break must write \n, which parses. // // That bound was 400 and 400 was too tight. Measured against Qwen3.5-0.8B: on // "почему гром слышно позже молнии?" the reply came back exactly 400 characters @@ -745,7 +756,7 @@ const phraseRepeatPenalty = 1.3 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} "\"" +string ::= "\"" ([^"\\\x00-\x1F] | "\\" ["\\/bfnrt] | "\\u" [0-9a-fA-F]{4}){0,1000} "\"" ws ::= [ \t\n]{0,4} ` @@ -1128,7 +1139,7 @@ func parseResponseMood(raw string) (response, mood string, err error) { return "", "", nil } var parsed responseMood - if e := json.Unmarshal([]byte(cleaned[start:end+1]), &parsed); e != nil { + if e := json.Unmarshal([]byte(escapeRawControls(cleaned[start:end+1])), &parsed); e != nil { if strings.HasPrefix(cleaned, "{") { return "", "", errBrokenJSON } @@ -1137,6 +1148,39 @@ func parseResponseMood(raw string) (response, mood string, err error) { return parsed.Response, parsed.Mood, nil } +// escapeRawControls escapes the control characters a model writes literally +// inside a JSON string, so a reply that is otherwise fine still parses. +// +// The grammar is what stops these being generated (Vikunja #537). This is the +// second line, for the paths that send no grammar at all — NoGrammar, and any +// remote model whose server ignores one. A raw newline is the shape that was +// measured; the rest of the range is here because the same argument covers it. +// Only characters outside a string are affected in principle, and JSON permits +// none of this range outside a string either, so escaping unconditionally +// cannot turn valid JSON into anything else. +func escapeRawControls(s string) string { + if !strings.ContainsFunc(s, func(r rune) bool { return r < 0x20 }) { + return s + } + var b strings.Builder + b.Grow(len(s) + 8) + for _, r := range s { + switch { + case r == '\n': + b.WriteString(`\n`) + case r == '\r': + b.WriteString(`\r`) + case r == '\t': + b.WriteString(`\t`) + case r < 0x20: + fmt.Fprintf(&b, `\u%04x`, r) + default: + b.WriteRune(r) + } + } + return b.String() +} + func parsePhrase(raw string) (body, summary string) { cleaned := strings.TrimSpace(raw) start := strings.Index(cleaned, "{") diff --git a/internal/router/llmrouter.go b/internal/router/llmrouter.go index 9be0906..270acf4 100644 --- a/internal/router/llmrouter.go +++ b/internal/router/llmrouter.go @@ -34,13 +34,20 @@ func NewLLMRouter(c Completer) *LLMRouter { return &LLMRouter{c: c} } // (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. +// +// The string class excludes the control range and the escape alternatives are +// exact, both for the reason the phrasing grammar gives (Vikunja #537): a raw +// newline inside a JSON string does not parse, and `"\\" .` licensed `\q`, +// which does not parse either. A route that does not parse falls through to the +// classifier, so here the defect reads as lost accuracy rather than as an empty +// reply. Same class as internal/phraser's responseGrammar, on purpose. const routeGrammar = ` root ::= "[" ws action ("," ws action)* ws "]" action ::= "{" ws "\"intent\"" ws ":" ws intent ("," ws field)* ws "}" intent ::= "\"fact\"" | "\"reminder\"" | "\"note\"" | "\"query\"" | "\"act\"" | "\"chat\"" | "\"system\"" | "\"unknown\"" field ::= key ws ":" ws string key ::= "\"key\"" | "\"value\"" | "\"text\"" | "\"verb\"" -string ::= "\"" ([^"\\] | "\\" .){0,120} "\"" +string ::= "\"" ([^"\\\x00-\x1F] | "\\" ["\\/bfnrt] | "\\u" [0-9a-fA-F]{4}){0,120} "\"" ws ::= [ \t\n]{0,4} ` diff --git a/internal/router/llmrouter_test.go b/internal/router/llmrouter_test.go index f82e016..24e7b00 100644 --- a/internal/router/llmrouter_test.go +++ b/internal/router/llmrouter_test.go @@ -38,9 +38,17 @@ func TestLLMRouterSetsRepeatPenalty(t *testing.T) { // An unbounded string rule lets one field eat the whole token budget. func TestRouteGrammarBoundsStrings(t *testing.T) { - if !strings.Contains(routeGrammar, `string ::= "\"" ([^"\\] | "\\" .){0,120} "\""`) { + if !strings.Contains(routeGrammar, `{0,120} "\""`) { t.Fatal("grammar string rule lost its length bound") } + // And it must not admit a raw newline or a made-up escape, either of which + // makes the route unparseable and costs the turn its router (Vikunja #537). + if !strings.Contains(routeGrammar, `[^"\\\x00-\x1F]`) { + t.Error("string rule admits raw control characters") + } + if strings.Contains(routeGrammar, `"\\" .`) { + t.Error(`string rule still admits "\\" . — \q satisfies the grammar and fails to parse`) + } } // A question naming a fact key used to be stored as a fact because the fact rule