diff --git a/cmd/mavend/clarify.go b/cmd/mavend/clarify.go index c9188d2..837933d 100644 --- a/cmd/mavend/clarify.go +++ b/cmd/mavend/clarify.go @@ -179,6 +179,20 @@ func (h *reactiveHandler) askClarify(ctx context.Context, dec router.Decision) ( // she parked — no second parser. If it still does not fill the gap she asks // again, up to MaxAttempts; after that she says out loud that she did not // understand. She never drops the request in silence. +// isOwnRequest reports whether an utterance asks for something in its own +// right, which is what a clarify answer never does. Two offline tests over +// tokens, both already written for other callers: a question shape, and a +// capture verb. Cheap on purpose — this runs on the answer to every parked +// question, and it must not cost a model call. +// +// It is not a general relevance test. A bare noun that answers nothing ("синий" +// after "Что сделать?") is still treated as an answer and still re-asked, and +// that is the intended shape: only an utterance that carries its own request +// wins over the question in front of it. +func isOwnRequest(text string) bool { + return router.IsQuestionShaped(text) || router.CarriesCaptureVerb(text) +} + func (h *reactiveHandler) resolveClarifyAnswer(ctx context.Context, text string) (string, bool) { if h.clarifyStore == nil { return "", false @@ -191,6 +205,23 @@ func (h *reactiveHandler) resolveClarifyAnswer(ctx context.Context, text string) intent := router.Intent(q.Intent) answer := h.extractor.Extract(ctx, intent, text, h.now()) merged := q.Answer(text, toDialogueSlots(answer)) + // He moved on. A parked question used to swallow whatever came next, so one + // act she could not fulfil ate the following three turns: "выключи свет в + // спальне" asked "Что сделать?", and "кто изобрёл телефон" was scored as an + // answer to it, then "как дела" after that (Vikunja #554). Nothing checked + // whether the words could be an answer at all. + // + // Deliberately narrow. It only fires where the answer filled nothing, so a + // turn that closes the gap is still an answer whatever shape it has, and + // the retry budget is untouched — the count was never the problem. Dropping + // the question and routing the utterance as itself is what he meant either + // way: if he really was answering, he can say it again, and if he was not, + // he gets the thing he asked for instead of being asked a third time. + if len(dialogue.StillMissing(q.Missing, merged)) > 0 && isOwnRequest(text) { + h.clarifyStore.Delete(dialogueIDOf(ctx)) + log.Printf("voice: clarify — %q is its own request, not an answer to %v; dropping the question", text, q.Missing) + return "", false + } // Fold a newly answered subject into the raw utterance. Downstream actions // phrase from Utterance, not from the text slot — actionReminder stores it // as the reminder payload — so a reminder clarified out of a bare "напомни" diff --git a/cmd/mavend/clarify_test.go b/cmd/mavend/clarify_test.go index 56b0722..66e235c 100644 --- a/cmd/mavend/clarify_test.go +++ b/cmd/mavend/clarify_test.go @@ -564,3 +564,59 @@ func TestARestartExpiresTheParkedQuestion(t *testing.T) { t.Fatalf("notice = %q, want silence: nothing survived to expire", notice) } } + +// TestClarifyStepsAsideForItsOwnRequest — Vikunja #554. An act she could not +// fulfil parked "Что сделать?", and the three turns after it were scored as +// answers to that question: a world question, then "как дела", then the give-up +// line. None of them was ever an answer. +func TestClarifyStepsAsideForItsOwnRequest(t *testing.T) { + ctx := context.Background() + h, _, _ := newClarifyHandler(t) + + if _, asked := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "выключи свет в спальне"}, "выключи свет в спальне")); !asked { + t.Fatal("an act with no fn should be asked about") + } + if reply, handled := h.resolveClarifyAnswer(ctx, "кто изобрёл телефон"); handled { + t.Fatalf("a world question must route as itself, got %q", reply) + } + if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil { + t.Error("the parked question must be dropped, not left to eat the turn after this one") + } +} + +// TestClarifyStillRetriesOnAnAnswerThatMissed — the other half of #554, and the +// reason the test above is narrow. A bare noun answers nothing either, but it +// carries no request of its own, so she asks again as before. +func TestClarifyStillRetriesOnAnAnswerThatMissed(t *testing.T) { + ctx := context.Background() + h, _, _ := newClarifyHandler(t) + + if _, asked := h.askClarify(ctx, clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме")); !asked { + t.Fatal("expected the time question") + } + reply, handled := h.resolveClarifyAnswer(ctx, "ага") + if !handled || reply == "" { + t.Fatalf("a missed answer must still be re-asked, handled=%v reply=%q", handled, reply) + } + if h.clarifyStore.Get(voiceDialogueID, h.now()) == nil { + t.Error("the question must survive a missed answer") + } +} + +// TestClarifyQuestionShapedAnswerThatFillsTheGapStillLands — the guard runs only +// where nothing was filled. "во сколько?" is question-shaped and is also how a +// time gets said back, so an answer that closes the gap wins whatever its shape. +func TestClarifyQuestionShapedAnswerThatFillsTheGapStillLands(t *testing.T) { + ctx := context.Background() + h, st, _ := newClarifyHandler(t) + + if _, asked := h.askClarify(ctx, clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме")); !asked { + t.Fatal("expected the time question") + } + if reply, handled := h.resolveClarifyAnswer(ctx, "а что если в 11:00"); !handled || reply == clarifyGaveUp { + t.Fatalf("an answer that fills the gap must land, handled=%v reply=%q", handled, reply) + } + if reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour)); err != nil || len(reminders) != 1 { + t.Fatalf("reminder was not created: reminders=%v err=%v", reminders, err) + } +} diff --git a/cmd/mavend/personalboundary.go b/cmd/mavend/personalboundary.go index d71af54..d239bdd 100644 --- a/cmd/mavend/personalboundary.go +++ b/cmd/mavend/personalboundary.go @@ -94,6 +94,15 @@ var worldSeeds = []string{ // topic seeds had already let them past the weather source. "какой сегодня курс валют", "что сегодня происходит в мире", + // The narrative shape (Vikunja #554). "расскажи про Байкал" was refused as + // his by 0.0052, and nothing here was phrased as an order rather than a + // question: every world seed above opens with an interrogative. So a world + // question that names its subject and asks for prose landed nearer "я тебе + // рассказывал об этом?", which is the same verb about his own words. + "расскажи про байкал", + "расскажи про древний рим", + "объясни как работает двигатель", + "tell me about the roman empire", } // personalBoundary holds the embedded seeds. Zero value is usable and means diff --git a/cmd/mavend/personalboundary_test.go b/cmd/mavend/personalboundary_test.go index 4c62047..cc21295 100644 --- a/cmd/mavend/personalboundary_test.go +++ b/cmd/mavend/personalboundary_test.go @@ -82,6 +82,13 @@ func TestONNXPersonalBoundary(t *testing.T) { {"какой сегодня праздник", false}, {"что интересного произошло сегодня в мире", false}, {"кто выиграл вчера матч", false}, + // The narrative shape, held out from the seeds above (Vikunja #554). + // The control is the row after them: the same verb about his own words + // is still his. + {"расскажи про эверест", false}, + {"расскажи про войну 1812 года", false}, + {"объясни что такое инфляция", false}, + {"я рассказывал тебе про байкал?", true}, } h := &reactiveHandler{recall: recallWiring{embedder: emb}} diff --git a/internal/phraser/llmphraser.go b/internal/phraser/llmphraser.go index 913210c..84d486b 100644 --- a/internal/phraser/llmphraser.go +++ b/internal/phraser/llmphraser.go @@ -512,19 +512,40 @@ func (p *LLMPhraser) PhraseQuery(ctx context.Context, utterance string, notes [] // message array from dialogue history + the current user utterance. On any LLM // error it returns both ChatFallback and the error, on the same rule as // PhraseQuery: the fallback keeps the turn alive, the error stays visible. +// chatUserMessage folds the prior turns and the current one into a single user +// message, because some chat templates (Ministral and others) reject two user +// turns in a row. That constraint is real; what was wrong is how it was met. +// +// The turns used to be joined with newlines and nothing else, so the model was +// handed four unlabelled lines and no way to tell which one it was answering +// (Vikunja #554). It answered an earlier one, or answered all of them at once: +// asked "как дела" after a question about the telephone, she carried on about +// the telephone. Four turns live for fifteen minutes, so the wrong line was +// often several minutes old. +// +// The history holds only his own utterances, never her replies, so the label +// says so and stays in the second person the persona requires. With no history +// the message is the utterance alone, which is the common case and unchanged. +func chatUserMessage(utterance string, history []dialogue.Turn) string { + prior := make([]string, 0, len(history)) + for _, t := range history { + if s := strings.TrimSpace(t.Text); s != "" { + prior = append(prior, "- "+s) + } + } + if len(prior) == 0 { + return strings.TrimSpace(utterance) + } + return "Раньше ты говорил:\n" + strings.Join(prior, "\n") + + "\n\nОтветь только на то, что ты говоришь сейчас: " + strings.TrimSpace(utterance) +} + func (p *LLMPhraser) PhraseChat(ctx context.Context, utterance string, history []dialogue.Turn) (string, error) { sys := chatSystemPrompt(p.cfg.ContextBlock) msgs := []chatMsg{ {Role: "system", Content: sys}, } - // Combine history and current utterance into one user message. - // Some model chat templates (Ministral, etc.) reject consecutive user turns. - var combined string - for _, t := range history { - combined += t.Text + "\n" - } - combined += utterance - msgs = append(msgs, chatMsg{Role: "user", Content: strings.TrimSpace(combined)}) + msgs = append(msgs, chatMsg{Role: "user", Content: chatUserMessage(utterance, history)}) resp, err := p.chatWithMessages(ctx, msgs, 768) if err != nil { diff --git a/internal/phraser/llmphraser_chatmsg_test.go b/internal/phraser/llmphraser_chatmsg_test.go new file mode 100644 index 0000000..466efb5 --- /dev/null +++ b/internal/phraser/llmphraser_chatmsg_test.go @@ -0,0 +1,42 @@ +package phraser + +import ( + "strings" + "testing" + + "github.com/kami/maven/internal/dialogue" +) + +// TestChatUserMessageMarksWhichTurnToAnswer — Vikunja #554. Four prior turns +// were joined with newlines and nothing else, so nothing in the message said +// which line was the question. +func TestChatUserMessageMarksWhichTurnToAnswer(t *testing.T) { + got := chatUserMessage("как дела", []dialogue.Turn{ + {Text: "кто изобрёл телефон"}, + {Text: "а когда это было"}, + }) + if !strings.Contains(got, "кто изобрёл телефон") { + t.Error("the prior turns must survive — they are what anaphora reads") + } + now := strings.LastIndex(got, "как дела") + if now < strings.Index(got, "кто изобрёл телефон") { + t.Error("the current utterance must come last, after the turns it follows") + } + if !strings.Contains(got, "Раньше ты говорил") { + t.Errorf("the prior turns must be labelled as prior: %q", got) + } + if strings.Contains(got, " вы ") || strings.Contains(got, "Вы ") { + t.Errorf("the persona addresses him informally: %q", got) + } +} + +// TestChatUserMessageWithNoHistoryIsJustTheUtterance — the common case must not +// grow a preamble that the model then has to see past. +func TestChatUserMessageWithNoHistoryIsJustTheUtterance(t *testing.T) { + if got := chatUserMessage(" привет ", nil); got != "привет" { + t.Errorf("chatUserMessage = %q, want %q", got, "привет") + } + if got := chatUserMessage("привет", []dialogue.Turn{{Text: " "}}); got != "привет" { + t.Errorf("a blank prior turn must not label anything: %q", got) + } +} diff --git a/internal/router/question.go b/internal/router/question.go index 356e0af..b50a050 100644 --- a/internal/router/question.go +++ b/internal/router/question.go @@ -26,6 +26,24 @@ var ( captureVerbs = lexicon.CaptureVerbs() ) +// CarriesCaptureVerb reports whether text tells Maven to write something down. +// Sibling of IsQuestionShaped and matched over the same tokens, and the two do +// not overlap: IsQuestionShaped returns false for anything this returns true +// for, because "запиши что я пил воду" is a capture and not a question. +// +// Both exist together so a caller can ask "is this its own request?" — a +// clarify answer that asks a question or orders a capture is not an answer +// (Vikunja #554). +func CarriesCaptureVerb(text string) bool { + toks := planTokens(strings.TrimSpace(text)) + for _, v := range captureVerbs { + if hasTok(toks, v) { + return true + } + } + return false +} + // IsQuestionShaped reports whether text asks for something rather than // records it. It is a deterministic offline test over tokens, so it costs // nothing and never depends on the model that produced the routing decision.