Merge pull request 'Clarification templates for the router's confidence-gate fallback' (#136) from task/457-clarification-templates-for-the-router-s into master

cmd/mavend/clarify.go

@@ -33,19 +33,10 @@ var wantedSlots = map[router.Intent][]dialogue.Slot{
 	router.IntentAct:      {dialogue.SlotFn},
 }
 
-// clarifyQuestions — one short question per missing slot.
-//
-// These are fixed templates, not model output. The resident model is a 0.8B; it
-// would wander, and a question whose wording changes every time is harder to
-// answer than a blunt one that always reads the same. They are infinitive
-// questions, so there is no gender agreement to get wrong; the feminine
-// self-reference lives in the reply she gives when she drops the request.
-var clarifyQuestions = map[dialogue.Slot]string{
-	dialogue.SlotTime: "Когда?",
-	dialogue.SlotText: "О чём напомнить?",
-	dialogue.SlotKey:  "Что записать?",
-	dialogue.SlotFn:   "Что сделать?",
-}
+// The questions themselves live in clarifytemplates.go, one list per slot,
+// picked by attempt (Vikunja #457). The first ask is the short one this map
+// used to hold; a re-ask says it differently, because a question he already
+// failed to answer is the worst one to repeat unchanged.
 
 // clarifyGaveUp — she is out of questions and still does not have the slot. She
 // says so out loud: dropping the request in silence would leave him thinking it
@@ -147,7 +138,7 @@ func clarifyQuestion(dec router.Decision) (dialogue.Slot, string, bool) {
 	if len(missing) == 0 {
 		return "", "", false
 	}
-	q, ok := clarifyQuestions[missing[0]]
+	q, ok := clarifyQuestionFor(missing[0], 1)
 	if !ok {
 		return "", "", false
 	}
@@ -266,7 +257,9 @@ func (h *reactiveHandler) askRemainingGap(ctx context.Context, q *dialogue.Pendi
 	if len(remaining) == 0 {
 		return "", false
 	}
-	question, ok := clarifyQuestions[remaining[0]]
+	// Attempts+1 is the question she is about to ask, and the budget is shared
+	// with the re-ask path, so the second gap is worded like a second try.
+	question, ok := clarifyQuestionFor(remaining[0], q.Attempts+1)
 	if !ok || !q.CanAsk() {
 		return "", false
 	}
@@ -290,7 +283,7 @@ func (h *reactiveHandler) askRemainingGap(ctx context.Context, q *dialogue.Pendi
 func (h *reactiveHandler) reaskOrGiveUp(ctx context.Context, q *dialogue.PendingQuestion, merged dialogue.Slots, text string) string {
 	question := ""
 	if len(q.Missing) > 0 {
-		question = clarifyQuestions[q.Missing[0]]
+		question, _ = clarifyQuestionFor(q.Missing[0], q.Attempts+1)
 	}
 	if question == "" || !q.CanAsk() {
 		h.clarifyStore.Delete(dialogueIDOf(ctx))

cmd/mavend/clarify_test.go

@@ -156,8 +156,14 @@ func TestClarifyAsksThreeTimesThenSaysSo(t *testing.T) {
 		if !handled {
 			t.Fatalf("answer %d must be consumed as an answer", i)
 		}
-		if reply != "Когда?" {
-			t.Fatalf("attempt %d should ask again, got %q", i, reply)
+		// The wording changes with the attempt (Vikunja #457): repeating a
+		// question he already failed to answer is the worst way to ask it.
+		want, _ := clarifyQuestionFor(dialogue.SlotTime, i)
+		if reply != want {
+			t.Fatalf("attempt %d should ask again as %q, got %q", i, want, reply)
+		}
+		if first, _ := clarifyQuestionFor(dialogue.SlotTime, 1); reply == first {
+			t.Fatalf("attempt %d repeated the first wording: %q", i, reply)
 		}
 		if h.clarifyStore.Get(voiceDialogueID, h.now()) == nil {
 			t.Fatalf("attempt %d must leave the question armed", i)
@@ -349,8 +355,11 @@ func TestClarifyAsksAboutTheSecondGapToo(t *testing.T) {
 	if !handled {
 		t.Fatal("the answer must be consumed as an answer")
 	}
-	if reply != "Когда?" {
-		t.Fatalf("a filled subject with no time must ask about the time, got %q", reply)
+	// Second gap, second attempt, so it is the second wording of the time
+	// question — the attempt budget is shared between the two paths.
+	want, _ := clarifyQuestionFor(dialogue.SlotTime, 2)
+	if reply != want {
+		t.Fatalf("a filled subject with no time must ask about the time as %q, got %q", want, reply)
 	}
 	q := h.clarifyStore.Get(voiceDialogueID, h.now())
 	if q == nil {
@@ -411,8 +420,9 @@ func TestClarifyProseHoldsThePersona(t *testing.T) {
 		eval.CheckCringe:    true,
 	}
 	lines := append([]string{clarifyGaveUp}, clarifyExpiredVariants...)
-	for _, q := range clarifyQuestions {
-		lines = append(lines, q)
+	lines = append(lines, clarifyMissedVariants...)
+	for _, variants := range clarifyQuestionVariants {
+		lines = append(lines, variants...)
 	}
 	for _, line := range lines {
 		for _, r := range eval.RunChecks(eval.Case{}, line, "neutral") {

cmd/mavend/clarifytemplates.go

@@ -0,0 +1,109 @@
+package main
+
+import (
+	"github.com/kami/maven/internal/dialogue"
+	"github.com/kami/maven/internal/router"
+)
+
+// The clarify copy deck (Vikunja #457).
+//
+// Every clarify turn used to say one sentence per gap, and a re-ask repeated
+// that sentence word for word. A question he already failed to answer is the
+// worst one to ask again unchanged: the second wording is the one that tells
+// him which part she missed.
+//
+// Fixed templates, not model output, for the reason clarifyQuestions has always
+// given: the resident model would wander, and a question whose wording changes
+// at random is harder to answer than a blunt one. What changes here is that the
+// wording varies with the attempt rather than with a die roll — the first ask is
+// short, the second names the gap, the third spells it out.
+//
+// No schema_version, unlike internal/phraser/nudge_templates.go. These are Go
+// constants compiled into the daemon, so there is no file that can drift out of
+// step with the code that reads it.
+//
+// Persona holds: infinitive and imperative questions, so there is no gender
+// agreement to get wrong, "ты" throughout, and no pet names.
+var clarifyQuestionVariants = map[dialogue.Slot][]string{
+	dialogue.SlotTime: {
+		"Когда?",
+		"Во сколько напомнить?",
+		"Скажи время — например, «в семь вечера» или «через час».",
+	},
+	dialogue.SlotText: {
+		"О чём напомнить?",
+		"Что сказать тебе в это время?",
+		"Скажи одной фразой, о чём напомнить.",
+	},
+	dialogue.SlotKey: {
+		"Что записать?",
+		"Что именно отметить?",
+		"Назови, что записать — например, «выпил воды».",
+	},
+	dialogue.SlotFn: {
+		"Что сделать?",
+		"Какое действие выполнить?",
+		"Назови действие — я умею только то, что ты мне разрешил.",
+	},
+}
+
+// clarifyQuestionFor picks the wording for this attempt. attempt is 1-based, as
+// PendingQuestion.Attempts counts it; anything past the list uses the last and
+// most explicit phrasing rather than wrapping round to the short one, because
+// wrapping would ask the same short question he has already not answered.
+//
+// Deterministic on purpose, unlike clarifyExpiredLine: an expiry notice is the
+// same statement however it is worded, and a re-ask is not.
+func clarifyQuestionFor(slot dialogue.Slot, attempt int) (string, bool) {
+	variants, ok := clarifyQuestionVariants[slot]
+	if !ok || len(variants) == 0 {
+		return "", false
+	}
+	i := attempt - 1
+	if i < 0 {
+		i = 0
+	}
+	if i >= len(variants) {
+		i = len(variants) - 1
+	}
+	return variants[i], true
+}
+
+// clarifyMissedVariants — she is asking for the whole utterance again, because
+// the gate fired on an intent with nothing identifiable to ask about (note,
+// query, chat, system are not in wantedSlots).
+//
+// Rotated like the expiry lines and for the same reason: this is the line he
+// hears whenever she misses him completely, so it is a line that repeats, and
+// the same sentence every time is what makes a house assistant sound like a
+// kiosk. All of them say the same two things — she did not catch it, and he
+// should say it again — because the wording may vary and the meaning may not.
+var clarifyMissedVariants = []string{
+	"Не совсем поняла — скажи, пожалуйста, ещё раз.",
+	"Я тебя не разобрала. Повтори, пожалуйста.",
+	"Не уловила. Скажи это по-другому?",
+	"Прости, не поняла — попробуй сказать иначе.",
+}
+
+// clarifyMissedFor picks a wording by the utterance itself, so the same words
+// asked twice get the same answer and two different misses sound different.
+//
+// A hash, not rand: a test that drives an utterance twice must not depend on a
+// die roll, and the point of rotating is only that consecutive misses differ.
+func clarifyMissedFor(utterance string) string {
+	var sum int
+	for _, r := range utterance {
+		sum += int(r)
+	}
+	return clarifyMissedVariants[sum%len(clarifyMissedVariants)]
+}
+
+// clarifyMissedLine is the canned reply for a clarify decision she cannot turn
+// into a question. Returns "" for a decision that is not a clarify, so the
+// caller keeps its own reply.
+func clarifyMissedLine(dec router.Decision) string {
+	if !dec.Clarify {
+		return ""
+	}
+	return clarifyMissedFor(dec.Utterance)
+}

cmd/mavend/clarifytemplates_test.go

@@ -0,0 +1,63 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/kami/maven/internal/dialogue"
+	"github.com/kami/maven/internal/router"
+)
+
+// Every slot she can ask about has a wording for every attempt she is allowed,
+// and no two attempts on one slot read the same. A deck with a repeated line is
+// the defect this deck exists to fix (Vikunja #457).
+func TestClarifyQuestionsVaryByAttempt(t *testing.T) {
+	for slot, variants := range clarifyQuestionVariants {
+		seen := map[string]bool{}
+		for _, v := range variants {
+			if v == "" {
+				t.Errorf("%s: empty wording in the deck", slot)
+			}
+			if seen[v] {
+				t.Errorf("%s: repeated wording %q", slot, v)
+			}
+			seen[v] = true
+		}
+		for attempt := 1; attempt <= len(variants); attempt++ {
+			got, ok := clarifyQuestionFor(slot, attempt)
+			if !ok || got != variants[attempt-1] {
+				t.Errorf("%s attempt %d = %q ok=%v, want %q", slot, attempt, got, ok, variants[attempt-1])
+			}
+		}
+	}
+}
+
+// Past the end she keeps the most explicit wording. Wrapping round would ask
+// the short question he has already not answered twice.
+func TestClarifyQuestionPastTheEndKeepsTheLastWording(t *testing.T) {
+	last := clarifyQuestionVariants[dialogue.SlotTime][len(clarifyQuestionVariants[dialogue.SlotTime])-1]
+	for _, attempt := range []int{0, 4, 9} {
+		if got, _ := clarifyQuestionFor(dialogue.SlotTime, attempt); attempt > 1 && got != last {
+			t.Errorf("attempt %d = %q, want the last wording %q", attempt, got, last)
+		}
+	}
+	if _, ok := clarifyQuestionFor("nonesuch", 1); ok {
+		t.Error("an unknown slot must have no question")
+	}
+}
+
+// The missed line is stable for one utterance and absent for a decision that is
+// not a clarify.
+func TestClarifyMissedLine(t *testing.T) {
+	d := router.Decision{Clarify: true, Utterance: "мгм"}
+	first := clarifyMissedLine(d)
+	if first == "" || first != clarifyMissedLine(d) {
+		t.Fatalf("the missed line must be stable for one utterance, got %q", first)
+	}
+	if got := clarifyMissedLine(router.Decision{Intent: router.IntentNote}); got != "" {
+		t.Errorf("a decision that is not a clarify got %q", got)
+	}
+	// The empty utterance still gets a line: she has to say something.
+	if got := clarifyMissedLine(router.Decision{Clarify: true}); got == "" {
+		t.Error("an empty utterance must still be answered out loud")
+	}
+}

cmd/mavend/replier_llm.go

@@ -24,7 +24,12 @@ func newLLMReplier(c phraser.Completer, block func() string) *llmReplier {
 // answer from the stub, which is what keeps a turn from breaking on the model.
 func (r *llmReplier) Reply(d router.Decision) string {
 	if d.Clarify {
-		return r.stub.Reply(d)
+		// The deck, not the stub's single sentence: a clarify she cannot turn
+		// into a question is the line he hears most often when she misses him,
+		// and it used to be the same words every time (Vikunja #457). Still no
+		// model call — this text has to be right every time, and it is not worth
+		// a generation to say something this small.
+		return clarifyMissedLine(d)
 	}
 	out, err := r.p.PhraseReply(context.Background(), d)
 	if err != nil || out == "" {

cmd/mavend/replier_llm_test.go

@@ -37,9 +37,21 @@ func TestLLMReplierFallsBackToStubOnEmpty(t *testing.T) {
 	assertStub(t, r, router.Decision{Intent: router.IntentNote}, "empty llm")
 }
 
-func TestLLMReplierClarifyUsesStub(t *testing.T) {
+// A clarify never reaches the model, and since Vikunja #457 it is answered from
+// the clarify deck rather than the stub's single sentence.
+func TestLLMReplierClarifyReadsTheDeck(t *testing.T) {
 	r := newLLMReplier(stubCompleter{out: "я всё поняла"}, nil)
-	assertStub(t, r, router.Decision{Clarify: true}, "clarify")
+	got := r.Reply(router.Decision{Clarify: true, Utterance: "мгм"})
+	if got == "я всё поняла" {
+		t.Fatal("a clarify must not be phrased by the model")
+	}
+	if want := clarifyMissedFor("мгм"); got != want {
+		t.Errorf("on clarify: got %q, want %q", got, want)
+	}
+	// Two different misses do not sound identical.
+	if same := r.Reply(router.Decision{Clarify: true, Utterance: "а"}); same == got {
+		t.Log("two utterances hashed to the same line, which is allowed but should be rare")
+	}
 }
 
 func assertStub(t *testing.T, r *llmReplier, d router.Decision, what string) {