diff --git a/cmd/mavend/clarify.go b/cmd/mavend/clarify.go index 4d4a3f1..22266e7 100644 --- a/cmd/mavend/clarify.go +++ b/cmd/mavend/clarify.go @@ -40,9 +40,10 @@ var clarifyQuestions = map[dialogue.Slot]string{ dialogue.SlotFn: "Что сделать?", } -// clarifyDropped — she asked once, the answer still did not fill the gap, so -// the request is gone. Said plainly, once, with no second question. -const clarifyDropped = "Не разобрала — скажи целиком, пожалуйста." +// 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 +// landed. Feminine self-reference ("поняла"), as everywhere. +const clarifyGaveUp = "Прости, я не поняла. Скажи, пожалуйста, по-другому." // missingFor returns the slots a decision still needs, most important first. // Empty ⇒ there is nothing identifiable to ask about. @@ -79,13 +80,14 @@ func (h *reactiveHandler) askClarify(dec router.Decision) (string, bool) { return "", false } h.clarifyStore.Put(voiceDialogueID, &dialogue.PendingQuestion{ - Intent: dialogue.Intent(dec.Intent), - Slots: toDialogueSlots(dec.Slots), - Missing: []dialogue.Slot{slot}, - Utterance: dec.Utterance, - Asked: h.now(), - TTL: clarifyTTL, - Attempts: 1, // asked once; MaxAttempts is 1, so there is no second ask + Intent: dialogue.Intent(dec.Intent), + Slots: toDialogueSlots(dec.Slots), + Missing: []dialogue.Slot{slot}, + Utterance: dec.Utterance, + Asked: h.now(), + TTL: clarifyTTL, + Attempts: 1, // this ask + MaxAttempts: h.clarifyMaxAttempts, }) log.Printf("voice: clarify — asked about %s for intent=%s", slot, dec.Intent) return question, true @@ -97,8 +99,9 @@ func (h *reactiveHandler) askClarify(dec router.Decision) (string, bool) { // resolveConfirm and checked in the same place. // // The answer is parsed with the same extractor the router uses, for the intent -// she parked — no second parser. If it still does not fill the gap the request -// is dropped: she does not ask again. +// 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. func (h *reactiveHandler) resolveClarifyAnswer(ctx context.Context, text string) (string, bool) { if h.clarifyStore == nil { return "", false @@ -107,17 +110,14 @@ func (h *reactiveHandler) resolveClarifyAnswer(ctx context.Context, text string) if q == nil { return "", false } - // One shot either way: the question is consumed whether or not the answer - // works, so a failed answer can't leave the question armed. - h.clarifyStore.Delete(voiceDialogueID) intent := router.Intent(q.Intent) answer := h.extractor.Extract(ctx, intent, text, h.now()) merged := q.Answer(text, toDialogueSlots(answer)) if len(dialogue.StillMissing(q.Missing, merged)) > 0 { - log.Printf("voice: clarify — answer %q did not fill %v, dropping", text, q.Missing) - return clarifyDropped, true + return h.reaskOrGiveUp(q, merged, text), true } + h.clarifyStore.Delete(voiceDialogueID) // Rebuild the decision as if it had routed cleanly, then run it down the // normal path. Clarify is deliberately false and the intent is unchanged: @@ -133,6 +133,29 @@ func (h *reactiveHandler) resolveClarifyAnswer(ctx context.Context, text string) return h.finishClarified(ctx, dec), true } +// reaskOrGiveUp handles an answer that left the gap open: ask the same question +// again while she has attempts left, otherwise say she did not understand and +// let the request go. Never returns "" — a mute give-up reads as "done". +func (h *reactiveHandler) reaskOrGiveUp(q *dialogue.PendingQuestion, merged dialogue.Slots, text string) string { + question := "" + if len(q.Missing) > 0 { + question = clarifyQuestions[q.Missing[0]] + } + if question == "" || !q.CanAsk() { + h.clarifyStore.Delete(voiceDialogueID) + log.Printf("voice: clarify — gave up on %v after %d question(s), answer was %q", q.Missing, q.Attempts, text) + return clarifyGaveUp + } + // Re-park with whatever the answer DID give, the clock restarted and one + // more question spent. + q.Slots = merged + q.Attempts++ + q.Asked = h.now() + h.clarifyStore.Put(voiceDialogueID, q) + log.Printf("voice: clarify — answer %q did not fill %v, asking again (attempt %d)", text, q.Missing, q.Attempts) + return question +} + // finishClarified runs a completed decision through the same steps a freshly // routed one takes: remember the turn, act, then phrase. func (h *reactiveHandler) finishClarified(ctx context.Context, dec router.Decision) string { @@ -146,6 +169,10 @@ func (h *reactiveHandler) finishClarified(ctx context.Context, dec router.Decisi if reply == "" { reply = h.replier.Reply(dec) } + if reply == "" { + // Belt: an empty reply here would be a silent drop. + reply = clarifyGaveUp + } return reply } diff --git a/cmd/mavend/clarify_test.go b/cmd/mavend/clarify_test.go index d83ec68..31178bc 100644 --- a/cmd/mavend/clarify_test.go +++ b/cmd/mavend/clarify_test.go @@ -86,7 +86,7 @@ func TestClarifyReminderCompletesOnAnswer(t *testing.T) { if !handled { t.Fatal("the answer to an open question must be consumed as an answer") } - if reply == clarifyDropped { + if reply == clarifyGaveUp { t.Fatalf("a good answer must not drop the request: %q", reply) } @@ -111,7 +111,7 @@ func TestClarifyFactCompletesOnAnswer(t *testing.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 == clarifyDropped { + 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" { @@ -137,26 +137,87 @@ func TestClarifyAnswerAfterTTLIsANewRequest(t *testing.T) { } } -// TestClarifyUnclearAnswerDropsWithoutAskingAgain — MaxAttempts is 1. -func TestClarifyUnclearAnswerDropsWithoutAskingAgain(t *testing.T) { +// 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 question") + 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 != clarifyDropped { - t.Fatalf("an unclear answer should drop the request, handled=%v reply=%q", handled, reply) + if !handled || reply != clarifyGaveUp { + t.Fatalf("the fourth try must give up out loud, handled=%v reply=%q", handled, reply) } - if strings.Contains(reply, "?") { - t.Fatalf("she must not ask a second question: %q", 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 dropped request must leave no armed question") + 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 dropped request must not create anything: reminders=%v err=%v", reminders, err) + 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) } } diff --git a/cmd/mavend/voice.go b/cmd/mavend/voice.go index 74c4bf6..2fde3ce 100644 --- a/cmd/mavend/voice.go +++ b/cmd/mavend/voice.go @@ -255,11 +255,13 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem dataStore: dataStore, dialogueSessions: dialogueSessions, clarifyStore: clarifyStore, - extractor: router.Extractor{Time: timeParser, Acts: matcher, Facts: router.DefaultFactParser{}}, - queryMinScore: cfg.Voice.QueryMinScore, - queryMinMargin: cfg.Voice.QueryMinMargin, - timeParser: timeParser, - ecosystem: eco, + // 0 here (unset config) ⇒ the dialogue default. + clarifyMaxAttempts: cfg.Voice.ClarifyMaxAttempts, + extractor: router.Extractor{Time: timeParser, Acts: matcher, Facts: router.DefaultFactParser{}}, + queryMinScore: cfg.Voice.QueryMinScore, + queryMinMargin: cfg.Voice.QueryMinMargin, + timeParser: timeParser, + ecosystem: eco, } // ----- the server (TCP listener) ----- @@ -318,6 +320,10 @@ type reactiveHandler struct { // clarify.go). nil ⇒ she falls back to the canned "не поняла" reply. clarifyStore *dialogue.ClarifyStore + // clarifyMaxAttempts — questions per request before she gives up out loud. + // 0 ⇒ dialogue.DefaultMaxAttempts (3). Set from VoiceConfig. + clarifyMaxAttempts int + // extractor parses the answer to an open question, with the same parsers // the router's own stage-2 uses. extractor router.Extractor diff --git a/internal/config/config.go b/internal/config/config.go index fd3c54f..4554bd7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -292,6 +292,10 @@ type VoiceConfig struct { // Negative ⇒ off. 0 ⇒ the default below. QueryMinMargin float64 `json:"query_min_margin,omitempty"` + // ClarifyMaxAttempts — how many clarifying questions she may ask about one + // request before she gives up and says she did not understand. Default 3. + ClarifyMaxAttempts int `json:"clarify_max_attempts,omitempty"` + // Persona — optional prompt prefix that tunes maven's character. Prepended // to every LLM system prompt (nudge phrasing, note queries, general // knowledge). Empty string ⇒ current hardcoded persona (feminine-gendered @@ -424,7 +428,9 @@ const ( // false recall from 5/5 to 1/5. Every larger delta costs real recall // without removing that last one until 0.020, which drops recall to 44%. DefaultQueryMinMargin = 0.008 - DefaultToolTimeout = 30 * time.Second + // DefaultClarifyMaxAttempts — see dialogue.DefaultMaxAttempts. + DefaultClarifyMaxAttempts = 3 + DefaultToolTimeout = 30 * time.Second // DefaultLLMRouter — route with the resident model unless told otherwise. DefaultLLMRouter = true @@ -516,6 +522,9 @@ func (c *Config) applyDefaults() { case c.Voice.QueryMinMargin < 0: c.Voice.QueryMinMargin = 0 } + if c.Voice.ClarifyMaxAttempts <= 0 { + c.Voice.ClarifyMaxAttempts = DefaultClarifyMaxAttempts + } if c.Voice.ToolTimeout <= 0 { c.Voice.ToolTimeout = Duration(DefaultToolTimeout) } diff --git a/internal/dialogue/clarify.go b/internal/dialogue/clarify.go index dc6c3c2..c91b06e 100644 --- a/internal/dialogue/clarify.go +++ b/internal/dialogue/clarify.go @@ -17,10 +17,11 @@ const ( SlotText Slot = "text" // Slots.Text ) -// MaxAttempts is 1 because Maven is not a nag (DESIGN.md § Non-goals). She asks -// one clarifying question. If the answer still leaves the slot empty she drops -// the request instead of asking again. -const MaxAttempts = 1 +// DefaultMaxAttempts — how many questions she may ask about one request. +// Three, because after three tries the likely problem is that she misheard the +// whole request, not one slot — so another question about that slot won't help. +// Configurable: voice.clarify_max_attempts. +const DefaultMaxAttempts = 3 // PendingQuestion is what Maven holds while she waits for an answer to an open // question. Unlike the yes/no confirms in cmd/mavend/voice.go, the answer here @@ -32,7 +33,17 @@ type PendingQuestion struct { Utterance string // the user's original raw words Asked time.Time TTL time.Duration - Attempts int // questions already asked; capped by MaxAttempts + Attempts int // questions already asked + // MaxAttempts caps Attempts. 0 ⇒ DefaultMaxAttempts. + MaxAttempts int +} + +// maxAttempts is MaxAttempts with the default filled in. +func (q *PendingQuestion) maxAttempts() int { + if q.MaxAttempts <= 0 { + return DefaultMaxAttempts + } + return q.MaxAttempts } func (q *PendingQuestion) IsExpired(now time.Time) bool { @@ -41,12 +52,9 @@ func (q *PendingQuestion) IsExpired(now time.Time) bool { // CanAsk reports whether Maven may ask another question about this request. func (q *PendingQuestion) CanAsk() bool { - return q.Attempts < MaxAttempts + return q.Attempts < q.maxAttempts() } -// TODO: the daemon will phrase the question text from Missing (one short ru -// question per Slot, feminine self-reference) and speak it here. - // ClarifyStore holds the parked questions. Same shape and locking as // SessionStore: keyed by dialogue id, expired entries dropped on read. type ClarifyStore struct { @@ -67,8 +75,7 @@ func NewClarifyStore(defaultTTL time.Duration) *ClarifyStore { } } -// TODO: the daemon will Put a question here when Decision.Clarify fires, in -// place of the flat "не разобрала" reply (cmd/mavend/voice.go). +// Put parks a question. Called on a clarify decision (cmd/mavend/clarify.go). func (s *ClarifyStore) Put(id string, q *PendingQuestion) { if q.TTL <= 0 { q.TTL = s.defaultTTL @@ -78,8 +85,7 @@ func (s *ClarifyStore) Put(id string, q *PendingQuestion) { s.mu.Unlock() } -// TODO: the daemon will Get on the next turn, parse that turn into Slots, call -// Answer, and Delete — the open-question twin of resolveConfirm. +// Get returns the live parked question, or nil when there is none. func (s *ClarifyStore) Get(id string, now time.Time) *PendingQuestion { s.mu.RLock() q, ok := s.questions[id] @@ -101,44 +107,45 @@ func (s *ClarifyStore) Delete(id string) { } // Answer merges the slots parsed from the user's answer into the parked ones. -// Only the slots listed in Missing are filled, and an already filled slot is -// never overwritten — the answer completes the original request, it does not -// restate it. Parsing the answer text into `answer` is the caller's job; this -// package must stay free of internal/router. +// Only the slots listed in Missing are touched. Within those, a value the answer +// carries WINS over what was parked: she asked about this slot, so «нет, в пять» +// after «в три» must replace the time, not be thrown away. +// +// This is the clarify answer only. A correction in a fresh turn ("вообще-то +// перенеси на пять") is a different code path (followUpMerge) — not here. +// +// Parsing the answer text into `answer` is the caller's job; this package must +// stay free of internal/router. func (q *PendingQuestion) Answer(text string, answer Slots) Slots { out := q.Slots for _, slot := range q.Missing { switch slot { case SlotTime: - if !out.HasTime && answer.HasTime { + if answer.HasTime { out.Time = answer.Time out.HasTime = true } case SlotKey: - if !out.HasKey && answer.HasKey { + if answer.HasKey { out.Key = answer.Key out.HasKey = true } case SlotValue: - if out.Value == "" && answer.Value != "" { + if answer.Value != "" { out.Value = answer.Value } case SlotFn: - if !out.HasFn && answer.HasFn { + if answer.HasFn { out.Fn = answer.Fn out.HasFn = true - if len(out.Args) == 0 { - out.Args = append([]string(nil), answer.Args...) - } + out.Args = append([]string(nil), answer.Args...) } case SlotText: - if out.Text == "" { - if answer.Text != "" { - out.Text = answer.Text - } else { - // No parse for a text slot — the raw answer IS the text. - out.Text = text - } + if answer.Text != "" { + out.Text = answer.Text + } else if out.Text == "" { + // No parse for a text slot — the raw answer IS the text. + out.Text = text } } } diff --git a/internal/dialogue/clarify_test.go b/internal/dialogue/clarify_test.go index 81d05ca..e9bec96 100644 --- a/internal/dialogue/clarify_test.go +++ b/internal/dialogue/clarify_test.go @@ -1,6 +1,7 @@ package dialogue import ( + "reflect" "testing" "time" ) @@ -89,12 +90,13 @@ func TestAnswerFillsOnlyMissingSlots(t *testing.T) { want: Slots{Text: "напомни позвонить", Time: answerTime, HasTime: true}, }, { - name: "does not overwrite a filled time", + // He restated it: «нет, в пять». The new value wins. + name: "a restated time overwrites the parked one", parked: Slots{Time: other, HasTime: true}, missing: []Slot{SlotTime}, - text: "в три", + text: "нет, в три", answer: Slots{Time: answerTime, HasTime: true}, - want: Slots{Time: other, HasTime: true}, + want: Slots{Time: answerTime, HasTime: true}, }, { name: "ignores slots that were not missing", @@ -121,12 +123,12 @@ func TestAnswerFillsOnlyMissingSlots(t *testing.T) { want: Slots{Fn: "restart", Args: []string{"nginx"}, HasFn: true}, }, { - name: "keeps existing args when fn was already known", + name: "a restated fn replaces the fn and its args", parked: Slots{Fn: "restart", Args: []string{"nginx"}, HasFn: true}, missing: []Slot{SlotFn}, text: "останови postgres", answer: Slots{Fn: "stop", Args: []string{"postgres"}, HasFn: true}, - want: Slots{Fn: "restart", Args: []string{"nginx"}, HasFn: true}, + want: Slots{Fn: "stop", Args: []string{"postgres"}, HasFn: true}, }, { name: "raw answer becomes the text when nothing was parsed", @@ -157,36 +159,34 @@ func TestAnswerFillsOnlyMissingSlots(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { q := &PendingQuestion{Slots: tc.parked, Missing: tc.missing, Asked: base} - got := q.Answer(tc.text, tc.answer) - if got.Time != tc.want.Time || got.HasTime != tc.want.HasTime || - got.Key != tc.want.Key || got.HasKey != tc.want.HasKey || - got.Value != tc.want.Value || got.Text != tc.want.Text || - got.Fn != tc.want.Fn || got.HasFn != tc.want.HasFn { + // Whole-struct compare: a new field in Slots is covered for free. + if got := q.Answer(tc.text, tc.answer); !reflect.DeepEqual(got, tc.want) { t.Fatalf("Answer = %+v, want %+v", got, tc.want) } - if len(got.Args) != len(tc.want.Args) { - t.Fatalf("Args = %v, want %v", got.Args, tc.want.Args) - } - for i := range got.Args { - if got.Args[i] != tc.want.Args[i] { - t.Fatalf("Args = %v, want %v", got.Args, tc.want.Args) - } - } }) } } -func TestCanAskCapsAtOneQuestion(t *testing.T) { - if MaxAttempts != 1 { - t.Fatalf("MaxAttempts = %d, want 1 (Maven asks once, she is not a nag)", MaxAttempts) +func TestCanAskAllowsThreeQuestionsByDefault(t *testing.T) { + if DefaultMaxAttempts != 3 { + t.Fatalf("DefaultMaxAttempts = %d, want 3", DefaultMaxAttempts) } - q := &PendingQuestion{Asked: base} - if !q.CanAsk() { - t.Fatal("a fresh question should be askable") + q := &PendingQuestion{Asked: base} // MaxAttempts unset ⇒ the default + for i := 0; i < 3; i++ { + if !q.CanAsk() { + t.Fatalf("question %d should be allowed", i+1) + } + q.Attempts++ } - q.Attempts = MaxAttempts if q.CanAsk() { - t.Fatal("the question should not be asked twice") + t.Fatal("a fourth question must not be allowed") + } +} + +func TestCanAskHonoursConfiguredMax(t *testing.T) { + q := &PendingQuestion{Asked: base, MaxAttempts: 1, Attempts: 1} + if q.CanAsk() { + t.Fatal("MaxAttempts 1 means one question only") } }