diff --git a/internal/phraser/failure_test.go b/internal/phraser/failure_test.go new file mode 100644 index 0000000..38d8ba9 --- /dev/null +++ b/internal/phraser/failure_test.go @@ -0,0 +1,72 @@ +package phraser + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// A dead server must be distinguishable from bad phrasing. Both PhraseChat and +// PhraseQuery keep the turn alive with canned text — "поговорили.", "не знаю.", +// "вот что я нашла: …" — and every one of those is also a legitimate reply, so +// the text alone cannot say which happened. The error is the only signal, and +// before Vikunja #397 it was dropped: the talk scorer reported a full run with +// zero errors off a server that answered nothing. +func TestPhrasingReportsTheFailureWithTheFallback(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "model not loaded", http.StatusServiceUnavailable) + })) + t.Cleanup(srv.Close) + p := NewLLMPhraserAt(srv.URL, Config{}) + + cases := []struct { + name string + call func() (string, error) + want string + }{ + {"chat", func() (string, error) { + return p.PhraseChat(context.Background(), "как дела", nil) + }, "поговорили."}, + {"knowledge", func() (string, error) { + return p.PhraseQuery(context.Background(), "кто написал войну и мир", nil) + }, "не знаю."}, + {"evidence", func() (string, error) { + return p.PhraseQuery(context.Background(), "сколько воды я выпил", []string{"два литра"}) + }, "вот что я нашла: два литра"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := c.call() + if err == nil { + t.Fatalf("no error from a dead server; the scorer would count this as bad phrasing") + } + if got != c.want { + t.Errorf("fallback text = %q, want %q — the daemon still has to say something", got, c.want) + } + }) + } +} + +// An empty answer is a failure too: the server is up and produced no tokens, +// which is not an answer and must not score as one. +func TestEmptyKnowledgeAnswerIsAnError(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{}) + + got, err := p.PhraseQuery(context.Background(), "кто написал войну и мир", nil) + if err == nil { + t.Fatal("an empty response scored as an answer") + } + if got != "не знаю." { + t.Errorf("fallback text = %q, want \"не знаю.\"", got) + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("error = %v; want it to name the empty response", err) + } +} diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index 2c5e28d..d5f7b31 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -428,8 +428,11 @@ func (p *LLMPhraser) PhraseNudge(ctx context.Context, c loop.Candidate) (deliver } // PhraseQuery prompts the LLM with the user's utterance and matching notes to -// compose a natural answer. Falls back to "вот что я нашла: " on any -// LLM error — better to give the raw data than silence. +// compose a natural answer. On any LLM error it returns BOTH the fallback text +// ("вот что я нашла: ", or "не знаю." with no notes) AND the error, so a +// caller that wants to keep the turn alive uses the text and a caller that is +// measuring counts the failure. Until Vikunja #397 the error was swallowed and a +// dead server scored as bad phrasing. func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes []string) (string, error) { // Blank sources are no sources. A caller that hands over one empty string — // a page that fetched to nothing, a snippet trimmed away — used to take the @@ -439,13 +442,15 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] if len(notes) == 0 { sys, prompt := p.knowledgePrompt(utterance) resp, err := p.chatWithSystem(ctx, sys, prompt, 768) - if err != nil || resp == "" { - return "не знаю.", nil + if err != nil { + return "не знаю.", fmt.Errorf("phrase query (knowledge): %w", err) + } + if resp == "" { + return "не знаю.", errEmptyResponse } text, _, perr := parseResponseMood(resp) if perr != nil { - log.Printf("phraser: PhraseQuery: %v", perr) - return "не знаю.", nil + return "не знаю.", fmt.Errorf("phrase query (knowledge): %w", perr) } if text != "" { return text, nil @@ -457,13 +462,12 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] 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) + cause := err + if cause == nil { + cause = perr } - if len(notes) == 1 { - return "вот что я нашла: " + notes[0], nil - } - return "вот что я нашла: " + strings.Join(notes, "; "), nil + return "вот что я нашла: " + strings.Join(notes, "; "), + fmt.Errorf("phrase query (evidence): %w", cause) } if text != "" { return text, nil @@ -472,8 +476,9 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] } // PhraseChat uses the LLM to respond conversationally, building a multi-turn -// message array from dialogue history + the current user utterance. Falls back -// to a simple greeting on any LLM error — better to say something than nothing. +// message array from dialogue history + the current user utterance. On any LLM +// error it returns both "поговорили." and the error, on the same rule as +// PhraseQuery: the fallback keeps the turn alive, the error stays visible. func (p *LLMPhraser) PhraseChat(ctx context.Context, utterance string, history []dialogue.Turn) (string, error) { sys := chatSystemPrompt(p.cfg.ContextBlock) msgs := []chatMsg{ @@ -490,13 +495,11 @@ func (p *LLMPhraser) PhraseChat(ctx context.Context, utterance string, history [ resp, err := p.chatWithMessages(ctx, msgs, 768) if err != nil { - log.Printf("phraser: PhraseChat: %v", err) - return "поговорили.", nil + return "поговорили.", fmt.Errorf("phrase chat: %w", err) } text, _, perr := parseResponseMood(resp) if perr != nil { - log.Printf("phraser: PhraseChat: %v", perr) - return "поговорили.", nil + return "поговорили.", fmt.Errorf("phrase chat: %w", perr) } if text != "" { return text, nil diff --git a/internal/phraser/swap_test.go b/internal/phraser/swap_test.go index aea6f39..01aa8e0 100644 --- a/internal/phraser/swap_test.go +++ b/internal/phraser/swap_test.go @@ -215,10 +215,12 @@ func TestSwap_RollbackFailureLeavesNoBackendAndDegrades(t *testing.T) { if _, _, aerr := p.acquire(); !errors.Is(aerr, ErrNoBackend) { t.Errorf("acquire error = %v; want ErrNoBackend", aerr) } - // Phrasing degrades to its fallback instead of failing the turn. + // Phrasing degrades to its fallback instead of failing the turn, and since + // Vikunja #397 it reports the error next to that fallback so a measuring + // caller can tell "no model" from "bad phrasing". got, err := p.PhraseChat(context.Background(), "привет", nil) - if err != nil { - t.Fatalf("PhraseChat after a total failure returned an error: %v", err) + if !errors.Is(err, ErrNoBackend) { + t.Errorf("PhraseChat error = %v; want ErrNoBackend alongside the fallback", err) } if got == "" { t.Error("PhraseChat returned empty; the fallback must still say something") diff --git a/internal/phraser/world.go b/internal/phraser/world.go index a021a4b..b9ebc1e 100644 --- a/internal/phraser/world.go +++ b/internal/phraser/world.go @@ -37,6 +37,11 @@ type Remote interface { // (docs/evals/2026-08-02-workstation-gemma4-12b.md). var ErrNoWorldModel = errors.New("phraser: no world model available") +// errEmptyResponse — the server answered and said nothing. A separate error from +// a transport failure because it means the model is up and produced no tokens, +// which is still not an answer and must not score as one. +var errEmptyResponse = errors.New("phraser: empty response from the model") + // chatTemperature — what the phraser's own transport has always sampled at. // Named so the remote path cannot drift from it silently. Whether 0.7 is right // at all is Vikunja #402, and answering that here would hide a phrasing change