diff --git a/cmd/mavend/clarify.go b/cmd/mavend/clarify.go index 35f8573..6220725 100644 --- a/cmd/mavend/clarify.go +++ b/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 - // when she has something to run, and say what it is (Vikunja #556). + // An act she could not resolve is a refusal, not a question (Vikunja #556). if slot == dialogue.SlotFn { - var allow []string - if h.matcher != nil { - 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 + log.Printf("voice: clarify — act %q matched no capability; saying so instead of asking", dec.Utterance) + return actNotRecognized, true } h.clarifyStore.Put(dialogueIDOf(ctx), &dialogue.PendingQuestion{ Intent: dialogue.Intent(dec.Intent), diff --git a/cmd/mavend/clarify_test.go b/cmd/mavend/clarify_test.go index 904dbac..84228e3 100644 --- a/cmd/mavend/clarify_test.go +++ b/cmd/mavend/clarify_test.go @@ -231,10 +231,10 @@ func TestClarifyRestatedAnswerWins(t *testing.T) { } } -// TestClarifiedActOffAllowlistIsStillRefused — clarification fills in an -// argument, it never grants authority. One tool is enabled so there is a -// question to park at all (Vikunja #556); the answer names something else. -func TestClarifiedActOffAllowlistIsStillRefused(t *testing.T) { +// TestActOffAllowlistIsStillRefused — naming a capability is not being granted +// one. Since Vikunja #556 an unresolved act no longer parks a question, so this +// goes through applyAction, which is the only way an act runs. +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 { - t.Fatal("an act with no fn should be asked about") - } - reply, handled := h.resolveClarifyAnswer(ctx, "rm "+marker) - if !handled { - t.Fatal("the answer should be consumed") - } + reply := h.applyAction(ctx, router.Decision{ + Utterance: "rm " + marker, + Intent: router.IntentAct, + Slots: router.Slots{Fn: "rm " + marker, HasFn: true}, + }) 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 -// clarify path. -func TestClarifiedDestructiveActStillNeedsConfirm(t *testing.T) { +// TestDestructiveActStillNeedsConfirm — the confirm gate stands on the act path. +func TestDestructiveActStillNeedsConfirm(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 { - t.Fatal("expected a question") - } - reply, handled := h.resolveClarifyAnswer(ctx, "delete_backups") - if !handled { - t.Fatal("the answer should be consumed") - } + reply := h.applyAction(ctx, router.Decision{ + Utterance: "delete_backups", + Intent: router.IntentAct, + Slots: router.Slots{Fn: "delete_backups", HasFn: true}, + }) 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) - // 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) - } + 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") + // A reminder, not the act this bug was found on: since Vikunja #556 an act + // 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 -// enabled, "Что сделать?" has no answer he could give, so she says so and parks -// nothing rather than spending three turns on a request she cannot fulfil. -func TestActWithNothingOnTheOtherEndNamesTheGap(t *testing.T) { +// TestUnresolvedActSaysItDoesNotKnowTheCommand — Vikunja #556. "Что сделать?" +// has no answer he can give, so an act that matched no capability is refused in +// one line and nothing is parked. It does not recite what she can do instead. +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 == "" { - t.Fatal("an act with nothing on the other end must still say something") + if !spoken || reply != actNotRecognized { + t.Fatalf("reply = %q spoken=%v, want %q", reply, spoken, actNotRecognized) } - if strings.Contains(reply, "?") { - t.Errorf("reply = %q, want a statement, not a question", reply) + if strings.Contains(reply, "uptime") { + 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) - } -} diff --git a/cmd/mavend/clarifytemplates.go b/cmd/mavend/clarifytemplates.go index e0d64c6..99693db 100644 --- a/cmd/mavend/clarifytemplates.go +++ b/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 -// 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). +// actNotRecognized is what an act she cannot run gets (Vikunja #556). // -// The old deck asked "Что сделать?" whatever was on the other end. With an empty -// allowlist that question has no answer: nothing he says can match, so she asks, -// fails, asks again and gives up — three turns spent on a request she could have -// declined in the first one. So an empty allowlist names the gap and parks -// nothing, and a non-empty one asks a question he can actually answer by naming -// what she has. -func fnClarify(allow []string, attempt int) (reply string, ask bool) { - if len(allow) == 0 { - 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 -} +// The deck used to ask "Что сделать?" instead. That question has no answer he +// can give: he already said what he wanted, and nothing he repeats will match a +// capability that is not there. So she asked, failed, asked again and gave up — +// three turns spent on one refusal. She says it once now, and parks nothing. +// +// It does not recite the allowlist. A list of names he did not ask about is not +// an answer to the thing he did ask about. +const actNotRecognized = "Такую команду я не знаю." // clarifyQuestionFor picks the wording for this attempt. attempt is 1-based, as // PendingQuestion.Attempts counts it; anything past the list uses the last and