Merge: an unrecognized act says so and lists nothing (#205)

cmd/mavend/clarify.go

@@ -156,19 +156,10 @@ func (h *reactiveHandler) askClarify(ctx context.Context, dec router.Decision) (
 	if !ok {
 		return "", false
 	}
-	// An act is the one gap that may have nothing on the other end. Ask only
+	// An act she could not resolve is a refusal, not a question (Vikunja #556).
-	// when she has something to run, and say what it is (Vikunja #556).
 	if slot == dialogue.SlotFn {
-		var allow []string
+		log.Printf("voice: clarify — act %q matched no capability; saying so instead of asking", dec.Utterance)
-		if h.matcher != nil {
+		return actNotRecognized, true
-			allow = h.matcher.Allowlist()
-		}
-		reply, ask := fnClarify(allow, 1)
-		if !ask {
-			log.Printf("voice: clarify — act with %d allowlisted fns; naming the gap instead of asking", len(allow))
-			return reply, reply != ""
-		}
-		question = reply
 	}
 	h.clarifyStore.Put(dialogueIDOf(ctx), &dialogue.PendingQuestion{
 		Intent:      dialogue.Intent(dec.Intent),

cmd/mavend/clarify_test.go

@@ -231,10 +231,10 @@ func TestClarifyRestatedAnswerWins(t *testing.T) {
 	}
 }
 
-// TestClarifiedActOffAllowlistIsStillRefused — clarification fills in an
+// TestActOffAllowlistIsStillRefused — naming a capability is not being granted
-// argument, it never grants authority. One tool is enabled so there is a
+// one. Since Vikunja #556 an unresolved act no longer parks a question, so this
-// question to park at all (Vikunja #556); the answer names something else.
+// goes through applyAction, which is the only way an act runs.
-func TestClarifiedActOffAllowlistIsStillRefused(t *testing.T) {
+func TestActOffAllowlistIsStillRefused(t *testing.T) {
 	ctx := context.Background()
 	h, st, _ := newClarifyHandler(t)
 	marker := filepath.Join(t.TempDir(), "not-allowed-ran")
@@ -242,13 +242,11 @@ func TestClarifiedActOffAllowlistIsStillRefused(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if _, asked := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "сделай это"}, "сделай это")); !asked {
+	reply := h.applyAction(ctx, router.Decision{
-		t.Fatal("an act with no fn should be asked about")
+		Utterance: "rm " + marker,
-	}
+		Intent:    router.IntentAct,
-	reply, handled := h.resolveClarifyAnswer(ctx, "rm "+marker)
+		Slots:     router.Slots{Fn: "rm " + marker, HasFn: true},
-	if !handled {
+	})
-		t.Fatal("the answer should be consumed")
-	}
 	if strings.Contains(reply, "готово") {
 		t.Fatalf("an act that is not on the allowlist must not report success: %q", reply)
 	}
@@ -256,13 +254,12 @@ func TestClarifiedActOffAllowlistIsStillRefused(t *testing.T) {
 		t.Fatalf("a clarified act off the allowlist ran anyway: %v", err)
 	}
 	if tools, err := st.ListTools(ctx, "enabled"); err != nil || len(tools) != 1 {
-		t.Fatalf("clarify must not enable a tool: tools=%+v err=%v", tools, err)
+		t.Fatalf("an act must not enable a tool: tools=%+v err=%v", tools, err)
 	}
 }
 
-// TestClarifiedDestructiveActStillNeedsConfirm — the confirm gate survives the
+// TestDestructiveActStillNeedsConfirm — the confirm gate stands on the act path.
-// clarify path.
+func TestDestructiveActStillNeedsConfirm(t *testing.T) {
-func TestClarifiedDestructiveActStillNeedsConfirm(t *testing.T) {
 	ctx := context.Background()
 	h, st, _ := newClarifyHandler(t)
 	marker := filepath.Join(t.TempDir(), "destructive-ran")
@@ -270,18 +267,16 @@ func TestClarifiedDestructiveActStillNeedsConfirm(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if _, asked := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "сделай это"}, "сделай это")); !asked {
+	reply := h.applyAction(ctx, router.Decision{
-		t.Fatal("expected a question")
+		Utterance: "delete_backups",
-	}
+		Intent:    router.IntentAct,
-	reply, handled := h.resolveClarifyAnswer(ctx, "delete_backups")
+		Slots:     router.Slots{Fn: "delete_backups", HasFn: true},
-	if !handled {
+	})
-		t.Fatal("the answer should be consumed")
-	}
 	if !strings.Contains(reply, "да") || h.pending == nil {
-		t.Fatalf("a clarified destructive act must still park a confirm: reply=%q pending=%+v", reply, h.pending)
+		t.Fatalf("a destructive act must park a confirm: reply=%q pending=%+v", reply, h.pending)
 	}
 	if _, err := os.Stat(marker); !os.IsNotExist(err) {
-		t.Fatalf("a clarified destructive act ran before confirmation: %v", err)
+		t.Fatalf("a destructive act ran before confirmation: %v", err)
 	}
 }
 
@@ -575,14 +570,12 @@ func TestARestartExpiresTheParkedQuestion(t *testing.T) {
 // line. None of them was ever an answer.
 func TestClarifyStepsAsideForItsOwnRequest(t *testing.T) {
 	ctx := context.Background()
-	h, st, _ := newClarifyHandler(t)
+	h, _, _ := newClarifyHandler(t)
-	// One enabled tool, so there is a question to park (Vikunja #556).
-	if err := st.EnableTool(ctx, "uptime", []string{"true"}, false, "test", h.now()); err != nil {
-		t.Fatal(err)
-	}
 
-	if _, asked := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "выключи свет в спальне"}, "выключи свет в спальне")); !asked {
+	// A reminder, not the act this bug was found on: since Vikunja #556 an act
-		t.Fatal("an act with no fn should be asked about")
+	// no longer parks anything, so it can no longer eat the turn after it.
+	if _, asked := h.askClarify(ctx, clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме")); !asked {
+		t.Fatal("a reminder with no time should be asked about")
 	}
 	if reply, handled := h.resolveClarifyAnswer(ctx, "кто изобрёл телефон"); handled {
 		t.Fatalf("a world question must route as itself, got %q", reply)
@@ -629,62 +622,27 @@ func TestClarifyQuestionShapedAnswerThatFillsTheGapStillLands(t *testing.T) {
 	}
 }
 
-// TestActWithNothingOnTheOtherEndNamesTheGap — Vikunja #556. With no tool
+// TestUnresolvedActSaysItDoesNotKnowTheCommand — Vikunja #556. "Что сделать?"
-// enabled, "Что сделать?" has no answer he could give, so she says so and parks
+// has no answer he can give, so an act that matched no capability is refused in
-// nothing rather than spending three turns on a request she cannot fulfil.
+// one line and nothing is parked. It does not recite what she can do instead.
-func TestActWithNothingOnTheOtherEndNamesTheGap(t *testing.T) {
+func TestUnresolvedActSaysItDoesNotKnowTheCommand(t *testing.T) {
 	ctx := context.Background()
-	h, _, _ := newClarifyHandler(t)
+	h, st, _ := newClarifyHandler(t)
+	// Enabled tools change nothing here: this act matched none of them.
+	if err := st.EnableTool(ctx, "uptime", []string{"true"}, false, "test", h.now()); err != nil {
+		t.Fatal(err)
+	}
 
 	reply, spoken := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "выключи свет"}, "выключи свет"))
-	if !spoken || reply == "" {
+	if !spoken || reply != actNotRecognized {
-		t.Fatal("an act with nothing on the other end must still say something")
+		t.Fatalf("reply = %q spoken=%v, want %q", reply, spoken, actNotRecognized)
 	}
-	if strings.Contains(reply, "?") {
+	if strings.Contains(reply, "uptime") {
-		t.Errorf("reply = %q, want a statement, not a question", reply)
+		t.Errorf("reply = %q, want no list of capabilities he did not ask about", reply)
 	}
 	if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
 		t.Error("nothing to ask about, so nothing may be parked")
 	}
 }
 
-// TestActClarifyNamesWhatSheCanDo — the other half. With tools enabled the
-// question stands, and it names them so it is answerable.
-func TestActClarifyNamesWhatSheCanDo(t *testing.T) {
-	ctx := context.Background()
-	h, st, _ := newClarifyHandler(t)
-	for _, name := range []string{"uptime", "disk"} {
-		if err := st.EnableTool(ctx, name, []string{"true"}, false, "test", h.now()); err != nil {
-			t.Fatal(err)
-		}
-	}
 
-	reply, asked := h.askClarify(ctx, clarifyDec(router.IntentAct, router.Slots{Text: "сделай это"}, "сделай это"))
-	if !asked {
-		t.Fatal("with tools enabled she should still ask")
-	}
-	for _, want := range []string{"uptime", "disk"} {
-		if !strings.Contains(reply, want) {
-			t.Errorf("reply = %q, want it to name %q", reply, want)
-		}
-	}
-	if h.clarifyStore.Get(voiceDialogueID, h.now()) == nil {
-		t.Error("the request must be parked so the answer can land")
-	}
-}
-
-// TestFnClarifyCapsTheListItRecites — a spoken sentence cannot carry twelve
-// names, which is how many this deployment enables.
-func TestFnClarifyCapsTheListItRecites(t *testing.T) {
-	allow := []string{"a", "b", "c", "d", "e", "f", "g", "h"}
-	reply, ask := fnClarify(allow, 1)
-	if !ask {
-		t.Fatal("a non-empty allowlist is still a question")
-	}
-	if strings.Contains(reply, "g") || strings.Contains(reply, "h") {
-		t.Errorf("reply = %q, want the list capped at %d", reply, namedActsCap)
-	}
-	if !strings.Contains(reply, "…") {
-		t.Errorf("reply = %q, want it to admit the list was cut", reply)
-	}
-}

cmd/mavend/clarifytemplates.go

@@ -1,8 +1,6 @@
 package main
 
 import (
-	"strings"
-
 	"github.com/kami/maven/internal/dialogue"
 	"github.com/kami/maven/internal/router"
 )
@@ -42,6 +40,9 @@ var clarifyQuestionVariants = map[dialogue.Slot][]string{
 		"Что именно отметить?",
 		"Назови, что записать — например, «выпил воды».",
 	},
+	// Not spoken since Vikunja #556: askClarify answers actNotRecognized for a
+	// missing capability rather than asking. Kept because clarifyQuestion still
+	// reports the gap, and a re-ask deck with a hole in it is harder to read.
 	dialogue.SlotFn: {
 		"Что сделать?",
 		"Какое действие выполнить?",
@@ -49,35 +50,16 @@ var clarifyQuestionVariants = map[dialogue.Slot][]string{
 	},
 }
 
-// namedActsCap bounds how many capability names one question may recite. Six is
+// actNotRecognized is what an act she cannot run gets (Vikunja #556).
-// what a spoken sentence carries; past that the list stops being an answer and
-// becomes a wall he has to hold in his head.
-const namedActsCap = 6
-
-// fnClarify decides what to say about an act whose capability is missing, and
-// whether the request is worth parking (Vikunja #556).
 //
-// The old deck asked "Что сделать?" whatever was on the other end. With an empty
+// The deck used to ask "Что сделать?" instead. That question has no answer he
-// allowlist that question has no answer: nothing he says can match, so she asks,
+// can give: he already said what he wanted, and nothing he repeats will match a
-// fails, asks again and gives up — three turns spent on a request she could have
+// capability that is not there. So she asked, failed, asked again and gave up —
-// declined in the first one. So an empty allowlist names the gap and parks
+// three turns spent on one refusal. She says it once now, and parks nothing.
-// nothing, and a non-empty one asks a question he can actually answer by naming
+//
-// what she has.
+// It does not recite the allowlist. A list of names he did not ask about is not
-func fnClarify(allow []string, attempt int) (reply string, ask bool) {
+// an answer to the thing he did ask about.
-	if len(allow) == 0 {
+const actNotRecognized = "Такую команду я не знаю."
-		return "Я пока ничего не умею делать — мне не разрешён ни один инструмент.", false
-	}
-	q, ok := clarifyQuestionFor(dialogue.SlotFn, attempt)
-	if !ok {
-		return "", false
-	}
-	named := allow
-	tail := ""
-	if len(named) > namedActsCap {
-		named, tail = named[:namedActsCap], "…"
-	}
-	return q + " Я умею: " + strings.Join(named, ", ") + tail + ".", true
-}
 
 // clarifyQuestionFor picks the wording for this attempt. attempt is 1-based, as
 // PendingQuestion.Attempts counts it; anything past the list uses the last and