phrasing and routing: a raw newline is not JSON (V-537)

Sixty of the failures in the 2026-08-05 temperature sweep were one error, `phraser: model output starts as JSON but does not parse`, all of them in the reply family and two of them in all twelve runs. The write-up read that as truncation. It is not: no run hit the token cap. The string rule in both grammars was `[^"\\]`, which admits a literal newline. A model that wants two lines writes one, the generation satisfies the grammar, and json.Unmarshal then rejects it with "invalid character '\n' in string literal". The object starts with "{", so it came back as errBrokenJSON and the reply was an empty string. The router's rule also admitted `"\\" .`, so \q satisfied it and failed to parse the same way. Both string rules are now llama.cpp's own json.gbnf class: the control range is out and the escape alternatives are exact. Verified against the resident model on 8899 — llama-server accepts both grammars and both still emit what they did. escapeRawControls is the second line, for NoGrammar and for a remote server that ignores a grammar: a reply whose only fault is a raw newline is readable, so it is read rather than dropped.
2026-08-05 11:24:35 +04:00
parent fdc18edd87
commit 32d5f68710
5 changed files with 124 additions and 10 deletions
@@ -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)
+	}
+}
@@ -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")
 	}
 }

@@ -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, "{")
@@ -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}
 `

@@ -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