phraser: the reminder summary is cut in runes, and silence is an error (V-620)
Three defects in internal/phraser, all of the shape "reports done when
nothing happened".
The reminder summary was cut in bytes: `len(s) > 60` and `s[:57]`, in two
copies (Stub.PhraseReminder and LLMPhraser.PhraseReminder). On Russian a
letter is two bytes, so the cut fell at about 28 letters instead of 60 and
landed inside a letter about half the time. Sendable.Summary is what
voicesink hands to piper and what the telegram sink posts, so the half rune
was spoken and sent. One rune-counting helper now, shared by both. The test
that covered this was ASCII, which is what let the arithmetic stand.
The evidence branch of PhraseQuery and the bare-prose tail of PhraseChat
both returned ("", nil) when the server answered and the model wrote no
tokens. The knowledge branch has guarded that with errEmptyResponse since it
was written; these two did not. The daemon's callers check for the empty
string and paper over it, so the visible cost was the eval, which scored a
silent model as bad phrasing rather than as a failure, and a log line that
never appeared.
Stub.PhraseReminder set no Mood. Its sibling PhraseNudge sets "neutral" and
says in a comment why: the Stub is a production fallback and owes the output
contract a value. The zero value is not one of the five moods.
No prompt and no spoken wording changed, so the phrasing eval is unmoved.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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{
|
||||
|
||||
Reference in New Issue
Block a user