b2eb08bb51
runTurn computed the notice at step 2, after the confirm check had already returned. So he could be asked a question, walk off until it expired, come back and say "да" to a confirm that was still parked. The confirm answered and he never heard that the older request had been let go, even though the store had dropped it. Every other exit from runTurn carries the notice. The notice is now taken first and every early return wraps in withNotice, including the clarify answer path, where it is empty in practice because one dialogue id holds one question. Found in review of #50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TrVSBKe3RFDF4fGYKWYQnX
427 lines
18 KiB
Go
427 lines
18 KiB
Go
package main
|
||
|
||
import (
|
||
"context"
|
||
"os"
|
||
"path/filepath"
|
||
"strings"
|
||
"testing"
|
||
"time"
|
||
|
||
"github.com/kami/maven/internal/dialogue"
|
||
"github.com/kami/maven/internal/ipc"
|
||
"github.com/kami/maven/internal/router"
|
||
"github.com/kami/maven/internal/store"
|
||
"github.com/kami/maven/internal/tool"
|
||
"github.com/kami/maven/internal/voice"
|
||
)
|
||
|
||
// newClarifyHandler builds a handler with the clarify path wired and no model:
|
||
// stub date parser, the real fact parser, and a matcher over whatever tools the
|
||
// test enabled. `now` is fixed so TTL behaviour is testable.
|
||
func newClarifyHandler(t *testing.T) (*reactiveHandler, *store.Store, *time.Time) {
|
||
t.Helper()
|
||
st := newTestStore(t)
|
||
api := ipc.NewStoreAPI(st)
|
||
now := time.Date(2026, 7, 31, 9, 0, 0, 0, time.UTC)
|
||
matcher := tool.NewMatcher(api)
|
||
h := &reactiveHandler{
|
||
api: api,
|
||
dataStore: st,
|
||
tools: tool.NewExecutor(api, 2*time.Second),
|
||
matcher: matcher,
|
||
replier: voice.NewStubReplier(),
|
||
now: func() time.Time { return now },
|
||
dialogueSessions: dialogue.NewSessionStore(2 * time.Minute),
|
||
clarifyStore: dialogue.NewClarifyStore(clarifyTTL),
|
||
extractor: router.Extractor{
|
||
Time: router.StubDateTimeParser{},
|
||
Acts: matcher,
|
||
Facts: router.DefaultFactParser{},
|
||
},
|
||
}
|
||
return h, st, &now
|
||
}
|
||
|
||
func clarifyDec(intent router.Intent, slots router.Slots, utterance string) router.Decision {
|
||
return router.Decision{Utterance: utterance, Stage: 3, Intent: intent, Slots: slots, Clarify: true}
|
||
}
|
||
|
||
// TestClarifyQuestionForMissingSlot pins which question goes with which gap, and
|
||
// which intents get no question at all.
|
||
func TestClarifyQuestionForMissingSlot(t *testing.T) {
|
||
cases := []struct {
|
||
name string
|
||
dec router.Decision
|
||
want string
|
||
asked bool
|
||
}{
|
||
{"reminder without a time", clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме"), "Когда?", true},
|
||
{"fact without a key", clarifyDec(router.IntentFact, router.Slots{Text: "запиши"}, "запиши"), "Что записать?", true},
|
||
{"act without a fn", clarifyDec(router.IntentAct, router.Slots{Text: "сделай это"}, "сделай это"), "Что сделать?", true},
|
||
// A time with nothing to say at that time is still half a reminder, so
|
||
// the subject is what she asks about — not silence.
|
||
{"reminder that has a time but no subject", clarifyDec(router.IntentReminder, router.Slots{HasTime: true}, "напомни в 11"), "О чём напомнить?", true},
|
||
{"reminder that has both", clarifyDec(router.IntentReminder, router.Slots{Text: "позвонить маме", HasTime: true}, "напомни в 11 позвонить маме"), "", false},
|
||
{"chat is never worth a question", clarifyDec(router.IntentChat, router.Slots{Text: "мгм"}, "мгм"), "", false},
|
||
{"query is never worth a question", clarifyDec(router.IntentQuery, router.Slots{Text: "а"}, "а"), "", false},
|
||
}
|
||
for _, tc := range cases {
|
||
_, got, asked := clarifyQuestion(tc.dec)
|
||
if asked != tc.asked || got != tc.want {
|
||
t.Errorf("%s: got (%q, %v), want (%q, %v)", tc.name, got, asked, tc.want, tc.asked)
|
||
}
|
||
}
|
||
}
|
||
|
||
// TestClarifyReminderCompletesOnAnswer is the whole point of the feature: she
|
||
// asks for the missing time and the answer creates the reminder.
|
||
func TestClarifyReminderCompletesOnAnswer(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
|
||
question, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме"))
|
||
if !asked || question != "Когда?" {
|
||
t.Fatalf("expected the time question, got %q asked=%v", question, asked)
|
||
}
|
||
|
||
reply, handled := h.resolveClarifyAnswer(ctx, "в 11:00")
|
||
if !handled {
|
||
t.Fatal("the answer to an open question must be consumed as an answer")
|
||
}
|
||
if reply == clarifyGaveUp {
|
||
t.Fatalf("a good answer must not drop the request: %q", reply)
|
||
}
|
||
|
||
reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour))
|
||
if err != nil || len(reminders) != 1 {
|
||
t.Fatalf("clarified reminder was not created: reminders=%v err=%v", reminders, err)
|
||
}
|
||
if !strings.Contains(reminders[0].Payload, "маме") {
|
||
t.Fatalf("the reminder lost the original request: %q", reminders[0].Payload)
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("the question must be cleared once answered")
|
||
}
|
||
}
|
||
|
||
// TestClarifyFactCompletesOnAnswer — the fact path, where the answer carries
|
||
// both the key and the value.
|
||
func TestClarifyFactCompletesOnAnswer(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentFact, router.Slots{Text: "запиши"}, "запиши")); !asked {
|
||
t.Fatal("a fact with no key should be asked about")
|
||
}
|
||
if reply, handled := h.resolveClarifyAnswer(ctx, "пил воду"); !handled || reply == clarifyGaveUp {
|
||
t.Fatalf("answer should complete the fact, handled=%v reply=%q", handled, reply)
|
||
}
|
||
if fact, err := st.LatestFact(ctx, "water"); err != nil || fact.Key != "water" {
|
||
t.Fatalf("clarified fact was not written: fact=%+v err=%v", fact, err)
|
||
}
|
||
}
|
||
|
||
// TestClarifyAnswerAfterTTLIsANewRequest — a late answer is not an answer.
|
||
func TestClarifyAnswerAfterTTLIsANewRequest(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, now := newClarifyHandler(t)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни"}, "напомни")); !asked {
|
||
t.Fatal("expected a question")
|
||
}
|
||
*now = now.Add(clarifyTTL + time.Second)
|
||
|
||
if reply, handled := h.resolveClarifyAnswer(ctx, "в 11:00"); handled {
|
||
t.Fatalf("an answer past the TTL must fall through to normal routing, got %q", reply)
|
||
}
|
||
if reminders, err := st.DueReminders(ctx, now.Add(48*time.Hour)); err != nil || len(reminders) != 0 {
|
||
t.Fatalf("expired question must not create anything: reminders=%v err=%v", reminders, err)
|
||
}
|
||
}
|
||
|
||
// TestClarifyAsksThreeTimesThenSaysSo — three questions are allowed, the fourth
|
||
// is not, and running out is SPOKEN. Silence would read as "handled".
|
||
func TestClarifyAsksThreeTimesThenSaysSo(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни"}, "напомни")); !asked {
|
||
t.Fatal("expected a first question")
|
||
}
|
||
// Two more unclear answers ⇒ two more questions (3 asks in total).
|
||
for i := 2; i <= 3; i++ {
|
||
reply, handled := h.resolveClarifyAnswer(ctx, "ну не знаю")
|
||
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)
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) == nil {
|
||
t.Fatalf("attempt %d must leave the question armed", i)
|
||
}
|
||
}
|
||
|
||
reply, handled := h.resolveClarifyAnswer(ctx, "ну не знаю")
|
||
if !handled || reply != clarifyGaveUp {
|
||
t.Fatalf("the fourth try must give up out loud, handled=%v reply=%q", handled, reply)
|
||
}
|
||
if reply == "" || strings.Contains(reply, "?") {
|
||
t.Fatalf("giving up must be spoken and must not be another question: %q", reply)
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("a given-up request must leave no armed question")
|
||
}
|
||
if reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour)); err != nil || len(reminders) != 0 {
|
||
t.Fatalf("a given-up request must not create anything: reminders=%v err=%v", reminders, err)
|
||
}
|
||
}
|
||
|
||
// TestClarifyMaxAttemptsIsConfigurable — one question when the config says one.
|
||
func TestClarifyMaxAttemptsIsConfigurable(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, _, _ := newClarifyHandler(t)
|
||
h.clarifyMaxAttempts = 1
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни"}, "напомни")); !asked {
|
||
t.Fatal("expected a question")
|
||
}
|
||
if reply, handled := h.resolveClarifyAnswer(ctx, "ну не знаю"); !handled || reply != clarifyGaveUp {
|
||
t.Fatalf("with max 1 she must give up at once, handled=%v reply=%q", handled, reply)
|
||
}
|
||
}
|
||
|
||
// TestClarifyRestatedAnswerWins — «в 11:00», then «нет, в 15:00». The second
|
||
// value is the one that lands.
|
||
func TestClarifyRestatedAnswerWins(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни позвонить маме"}, "напомни позвонить маме")); !asked {
|
||
t.Fatal("expected a question")
|
||
}
|
||
// First answer parses, but re-park it by hand as if she had asked again:
|
||
// what matters here is that Answer prefers the newer value over the parked
|
||
// one, which is the case the daemon hits on a re-ask.
|
||
q := h.clarifyStore.Get(voiceDialogueID, h.now())
|
||
if q == nil {
|
||
t.Fatal("expected an armed question")
|
||
}
|
||
first := h.extractor.Extract(ctx, router.IntentReminder, "в 11:00", h.now())
|
||
q.Slots = q.Answer("в 11:00", toDialogueSlots(first))
|
||
|
||
if reply, handled := h.resolveClarifyAnswer(ctx, "нет, в 15:00"); !handled || reply == clarifyGaveUp {
|
||
t.Fatalf("the restated answer should complete the request, handled=%v reply=%q", handled, reply)
|
||
}
|
||
reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour))
|
||
if err != nil || len(reminders) != 1 {
|
||
t.Fatalf("expected one reminder: %v err=%v", reminders, err)
|
||
}
|
||
want := h.extractor.Extract(ctx, router.IntentReminder, "в 15:00", h.now())
|
||
if !reminders[0].FireTs.Equal(want.Time) {
|
||
t.Fatalf("reminder at %v, want the restated %v", reminders[0].FireTs, want.Time)
|
||
}
|
||
}
|
||
|
||
// TestClarifiedActOffAllowlistIsStillRefused — clarification fills in an
|
||
// argument, it never grants authority.
|
||
func TestClarifiedActOffAllowlistIsStillRefused(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
marker := filepath.Join(t.TempDir(), "not-allowed-ran")
|
||
|
||
if _, asked := h.askClarify(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")
|
||
}
|
||
if strings.Contains(reply, "готово") {
|
||
t.Fatalf("an act that is not on the allowlist must not report success: %q", reply)
|
||
}
|
||
if _, err := os.Stat(marker); !os.IsNotExist(err) {
|
||
t.Fatalf("a clarified act off the allowlist ran anyway: %v", err)
|
||
}
|
||
if tools, err := st.ListTools(ctx, "enabled"); err != nil || len(tools) != 0 {
|
||
t.Fatalf("clarify must not enable a tool: tools=%+v err=%v", tools, err)
|
||
}
|
||
}
|
||
|
||
// TestClarifiedDestructiveActStillNeedsConfirm — the confirm gate survives the
|
||
// clarify path.
|
||
func TestClarifiedDestructiveActStillNeedsConfirm(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
marker := filepath.Join(t.TempDir(), "destructive-ran")
|
||
if err := st.EnableTool(ctx, "delete_backups", []string{"touch", marker}, true, "test", h.now()); err != nil {
|
||
t.Fatal(err)
|
||
}
|
||
|
||
if _, asked := h.askClarify(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")
|
||
}
|
||
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)
|
||
}
|
||
if _, err := os.Stat(marker); !os.IsNotExist(err) {
|
||
t.Fatalf("a clarified destructive act ran before confirmation: %v", err)
|
||
}
|
||
}
|
||
|
||
// TestNoQuestionWhenNothingIsMissing — noise keeps the canned reply, so she
|
||
// never invents a question for nothing.
|
||
func TestNoQuestionWhenNothingIsMissing(t *testing.T) {
|
||
h, _, _ := newClarifyHandler(t)
|
||
for _, dec := range []router.Decision{
|
||
clarifyDec(router.IntentChat, router.Slots{Text: "эм"}, "эм"),
|
||
clarifyDec(router.IntentQuery, router.Slots{Text: "ммм"}, "ммм"),
|
||
clarifyDec(router.IntentNote, router.Slots{Text: "..."}, "..."),
|
||
} {
|
||
if question, asked := h.askClarify(dec); asked {
|
||
t.Fatalf("intent %s should keep the canned reply, got %q", dec.Intent, question)
|
||
}
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("noise must not park a question")
|
||
}
|
||
}
|
||
|
||
// TestClarifyExpiryIsAnnouncedAndWordsStillRoute — his answer lands after the
|
||
// TTL: she must say the old request is gone AND still answer the new words.
|
||
func TestClarifyExpiryIsAnnouncedAndWordsStillRoute(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, _, now := newClarifyHandler(t)
|
||
emb := router.NewHashEmbedder(1024)
|
||
h.embedder = emb
|
||
h.router = buildRouter(emb, h.matcher, 0.55, nil)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни"}, "напомни")); !asked {
|
||
t.Fatal("expected a question")
|
||
}
|
||
*now = now.Add(clarifyTTL + time.Second)
|
||
|
||
reply := h.handleText(ctx, "как дела")
|
||
if !isClarifyExpired(reply) {
|
||
t.Fatalf("expired question must be announced first, got %q", reply)
|
||
}
|
||
if trimClarifyExpired(reply) == "" {
|
||
t.Fatalf("the new words must still be answered, got only the notice: %q", reply)
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("the expired question must be gone")
|
||
}
|
||
// The notice is said once, not on every later utterance.
|
||
if reply := h.handleText(ctx, "как дела"); isClarifyExpired(reply) {
|
||
t.Fatalf("notice repeated on a later turn: %q", reply)
|
||
}
|
||
}
|
||
|
||
// TestNoPendingQuestionFallsThrough — with nothing parked, an utterance routes
|
||
// normally.
|
||
func TestNoPendingQuestionFallsThrough(t *testing.T) {
|
||
h, _, _ := newClarifyHandler(t)
|
||
if reply, handled := h.resolveClarifyAnswer(context.Background(), "напомни в 11:00"); handled {
|
||
t.Fatalf("no open question ⇒ must not be treated as an answer, got %q", reply)
|
||
}
|
||
}
|
||
|
||
// TestClarifyAsksAboutTheSecondGapToo — "напомни" with neither a subject nor a
|
||
// time. She asks about the subject, he gives it, and the request is still not
|
||
// complete. The old code handed applyAction a reminder with no time, which
|
||
// answered with a parse error for a question she never asked.
|
||
func TestClarifyAsksAboutTheSecondGapToo(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, st, _ := newClarifyHandler(t)
|
||
|
||
question, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{}, "напомни"))
|
||
if !asked || question != "О чём напомнить?" {
|
||
t.Fatalf("expected the subject question, got %q asked=%v", question, asked)
|
||
}
|
||
|
||
reply, handled := h.resolveClarifyAnswer(ctx, "позвонить маме")
|
||
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)
|
||
}
|
||
q := h.clarifyStore.Get(voiceDialogueID, h.now())
|
||
if q == nil {
|
||
t.Fatal("the second gap must leave a question armed")
|
||
}
|
||
if q.Slots.Text == "" {
|
||
t.Fatalf("the re-parked question lost the answered subject: %+v", q.Slots)
|
||
}
|
||
|
||
if reply, handled := h.resolveClarifyAnswer(ctx, "в 11:00"); !handled || reply == clarifyGaveUp {
|
||
t.Fatalf("the time answer must complete the reminder, handled=%v reply=%q", handled, reply)
|
||
}
|
||
reminders, err := st.DueReminders(ctx, h.now().Add(48*time.Hour))
|
||
if err != nil || len(reminders) != 1 {
|
||
t.Fatalf("expected one reminder: %v err=%v", reminders, err)
|
||
}
|
||
if !strings.Contains(reminders[0].Payload, "маме") {
|
||
t.Fatalf("the reminder lost the subject: %q", reminders[0].Payload)
|
||
}
|
||
}
|
||
|
||
// TestClarifySecondGapRespectsTheAttemptCap — the second gap spends a question
|
||
// out of the same budget, so it cannot turn a capped exchange into an endless
|
||
// one. With one attempt allowed she acts on what she has instead of asking.
|
||
func TestClarifySecondGapRespectsTheAttemptCap(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, _, _ := newClarifyHandler(t)
|
||
h.clarifyMaxAttempts = 1
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{}, "напомни")); !asked {
|
||
t.Fatal("expected the subject question")
|
||
}
|
||
reply, handled := h.resolveClarifyAnswer(ctx, "позвонить маме")
|
||
if !handled {
|
||
t.Fatal("the answer must be consumed")
|
||
}
|
||
if reply == "Когда?" {
|
||
t.Fatal("out of attempts she must not ask a second question")
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("no question may stay armed past the cap")
|
||
}
|
||
}
|
||
|
||
// TestExpiryNoticeSurvivesAConfirmTurn — she asks a question, he walks off, the
|
||
// question expires, he comes back and answers a confirm that is still parked.
|
||
// The confirm turn used to return before the notice was even computed, so he
|
||
// answered the confirm and never heard that the older request was let go.
|
||
func TestExpiryNoticeSurvivesAConfirmTurn(t *testing.T) {
|
||
ctx := context.Background()
|
||
h, _, now := newClarifyHandler(t)
|
||
|
||
if _, asked := h.askClarify(clarifyDec(router.IntentReminder, router.Slots{Text: "напомни"}, "напомни")); !asked {
|
||
t.Fatal("expected a question")
|
||
}
|
||
// A confirm parked with a longer life than the question, so only the
|
||
// question is stale when he speaks.
|
||
h.pending = &pendingAct{fn: "delete_backups", phrase: "удалить бэкапы", expiry: now.Add(time.Hour)}
|
||
*now = now.Add(clarifyTTL + time.Second)
|
||
|
||
reply := h.handleText(ctx, "нет")
|
||
if !isClarifyExpired(reply) {
|
||
t.Fatalf("the expired question must be announced on a confirm turn too, got %q", reply)
|
||
}
|
||
if trimClarifyExpired(reply) == "" {
|
||
t.Fatalf("the confirm answer must survive the notice, got only the notice: %q", reply)
|
||
}
|
||
if h.pending != nil {
|
||
t.Fatal("the confirm must still have been consumed")
|
||
}
|
||
if h.clarifyStore.Get(voiceDialogueID, h.now()) != nil {
|
||
t.Fatal("the expired question must be gone")
|
||
}
|
||
}
|