Merge V-554: three defects that made an ordinary conversation go wrong (#198)

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 "напомни"

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)
+	}
+}

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

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

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.
+	msgs = append(msgs, chatMsg{Role: "user", Content: chatUserMessage(utterance, history)})
-	// 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)})
 
 	resp, err := p.chatWithMessages(ctx, msgs, 768)
 	if err != nil {

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)
+	}
+}

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.