diff --git a/internal/phraser/failure_test.go b/internal/phraser/failure_test.go index 28636fe..0d06846 100644 --- a/internal/phraser/failure_test.go +++ b/internal/phraser/failure_test.go @@ -78,3 +78,36 @@ func TestEmptyKnowledgeAnswerIsAnError(t *testing.T) { t.Errorf("error = %v; want it to name the empty response", err) } } + +// The other two paths, which had no such guard. The evidence branch of +// PhraseQuery and PhraseChat both returned ("", nil) off a server that produced +// no tokens — an empty answer reported as a successful phrasing. The daemon's +// callers check for the empty string and paper over it; the eval does not, and +// scored a silent model as bad phrasing rather than as a failure. +func TestEmptyAnswerIsAnErrorOnEveryPath(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"choices":[{"message":{"content":""}}]}`)) + })) + t.Cleanup(srv.Close) + p := NewLLMPhraserAt(srv.URL, Config{}) + + t.Run("evidence", func(t *testing.T) { + got, err := p.PhraseQuery(context.Background(), "сколько воды я выпил", []string{"два литра"}) + if err == nil { + t.Fatal("an empty response scored as an answer") + } + if !isFallback(t, fbQuerySources, "два литра", got) { + t.Errorf("fallback text = %q, want a %q variant", got, fbQuerySources) + } + }) + t.Run("chat", func(t *testing.T) { + got, err := p.PhraseChat(context.Background(), "как дела", nil) + if err == nil { + t.Fatal("an empty response scored as an answer") + } + if !isFallback(t, fbChat, "", got) { + t.Errorf("fallback text = %q, want a %q variant", got, fbChat) + } + }) +} diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index d03d8e2..25ea6db 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -299,6 +299,16 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] if text != "" { return text, nil } + if raw == "" { + // Same guard the knowledge branch above has had since it was written, + // and this branch did not: the server answered and the model wrote + // nothing, which returned ("", nil) — an empty answer reported as a + // successful phrasing. The daemon's callers happen to check for the + // empty string, so it read as a silent fallback there; the eval scored + // it as bad phrasing rather than as the failure it is, and nothing on + // either path logged that the model had produced no tokens. + return SourcesFallback(strings.Join(notes, "; ")), errEmptyResponse + } return raw, nil } @@ -375,7 +385,13 @@ func (p *LLMPhraser) PhraseChat(ctx context.Context, utterance string, history [ if i := strings.IndexByte(resp, '\n'); i >= 0 { resp = resp[:i] } - return strings.TrimSpace(resp), nil + if resp = strings.TrimSpace(resp); resp == "" { + // The model was up and wrote nothing. Same rule as PhraseQuery: the + // fallback keeps the turn alive and the failure stays visible, rather + // than ("", nil) telling the caller the chat path succeeded. + return ChatFallback(), fmt.Errorf("phrase chat: %w", errEmptyResponse) + } + return resp, nil } func (p *LLMPhraser) PhraseReminder(ctx context.Context, d loop.ReminderDecision) (delivery.PhrasedReminder, error) { @@ -414,11 +430,9 @@ func (p *LLMPhraser) PhraseReminder(ctx context.Context, d loop.ReminderDecision if mood == "" { mood = "neutral" } - summary := body - if len(summary) > 60 { - summary = summary[:57] + "..." - } - return delivery.PhrasedReminder{Decision: d, Body: body, Summary: summary, Mood: mood}, nil + return delivery.PhrasedReminder{ + Decision: d, Body: body, Summary: reminderSummary(body), Mood: mood, + }, nil } func (p *LLMPhraser) chat(ctx context.Context, userPrompt string) (string, error) { diff --git a/internal/phraser/phraser.go b/internal/phraser/phraser.go index df184fc..ccf49d7 100644 --- a/internal/phraser/phraser.go +++ b/internal/phraser/phraser.go @@ -115,11 +115,32 @@ func (s *Stub) PhraseReminder(_ context.Context, d loop.ReminderDecision) (deliv if text == "" { text = "reminder" } - summary := text - if len(summary) > 60 { - summary = summary[:57] + "..." + // Mood, for the same reason PhraseNudge sets it: the Stub is a production + // fallback, so it owes the output contract a value. This one was left at the + // zero value, which is not one of the five moods. + return delivery.PhrasedReminder{ + Decision: d, Body: text, Summary: reminderSummary(text), Mood: "neutral", + }, nil +} + +// summaryLimit — how much of a reminder goes to the away channels. +const summaryLimit = 60 + +// reminderSummary shortens a reminder body to the away-channel summary. +// +// Counted in runes. Both copies of this counted bytes — `len(s) > 60` and +// `s[:57]` — and on a Russian reminder that is wrong twice. A Cyrillic letter is +// two bytes, so the cut fell at about 28 letters rather than 60; and byte 57 +// lands inside a letter about half the time, so the summary ended in half a +// rune. That is not cosmetic: Sendable.Summary is the text voicesink hands to +// piper and the text the telegram sink posts, so the broken byte was spoken and +// sent. +func reminderSummary(body string) string { + r := []rune(body) + if len(r) <= summaryLimit { + return body } - return delivery.PhrasedReminder{Decision: d, Body: text, Summary: summary}, nil + return string(r[:summaryLimit-3]) + "..." } // phraseNudge — the per-rule templates. each reads the context the predicate diff --git a/internal/phraser/phraser_test.go b/internal/phraser/phraser_test.go index 69bc2ea..e86c2e4 100644 --- a/internal/phraser/phraser_test.go +++ b/internal/phraser/phraser_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" "time" + "unicode/utf8" "github.com/kami/maven/internal/delivery" "github.com/kami/maven/internal/dialogue" @@ -185,6 +186,34 @@ func TestPhraseReminderTruncatesLongSummary(t *testing.T) { } } +// The same truncation, in the language she actually speaks. The test above is +// ASCII, which is what let the byte arithmetic stand: `len(s) > 60` and `s[:57]` +// cut a Russian reminder at about 28 letters instead of 60, and landed inside a +// letter about half the time. Summary is what voicesink hands to piper and what +// the telegram sink posts, so half a rune was spoken and sent. +func TestPhraseReminderSummaryCountsRunesNotBytes(t *testing.T) { + long := "позвонить маме и забрать посылку из пункта выдачи на соседней улице до восьми вечера" + rd := loop.ReminderDecision{ + Reminder: store.Reminder{Payload: `{"text":"` + long + `"}`}, + State: loop.State{Now: time.Now().UTC()}, + } + pr, _ := NewStub().PhraseReminder(context.Background(), rd) + if !utf8.ValidString(pr.Summary) { + t.Fatalf("summary is not valid UTF-8, it was cut mid-letter: %q", pr.Summary) + } + if n := utf8.RuneCountInString(pr.Summary); n > summaryLimit { + t.Fatalf("summary = %d runes, want at most %d: %q", n, summaryLimit, pr.Summary) + } + // The cut must be near the limit, not near half of it. A byte count would + // stop at 28 letters here. + if n := utf8.RuneCountInString(pr.Summary); n < summaryLimit-5 { + t.Fatalf("summary = %d runes, cut far too early — counted in bytes? %q", n, pr.Summary) + } + if pr.Mood == "" { + t.Error("Mood is empty; the Stub is a production fallback and owes the contract a mood") + } +} + func TestPhraseReminderNonJSONPayload(t *testing.T) { // a payload that isn't JSON → the phraser falls back to the raw string. rd := loop.ReminderDecision{