From 214a4032cfd6fc8226aa5cf202774973fc9ce4d0 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 12:51:47 +0400 Subject: [PATCH] Tell him when an expired clarify question is dropped Vikunja #382. A parked clarifying question past its TTL was discarded silently on read; now she says the old request is gone and the newly spoken words are still routed as a fresh utterance. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- cmd/mavend/clarify.go | 34 +++++++++++++++++++++++++++++ cmd/mavend/clarify_test.go | 30 ++++++++++++++++++++++++++ cmd/mavend/voice.go | 36 ++++++++++++++++++++----------- internal/dialogue/clarify.go | 15 +++++++++++++ internal/dialogue/clarify_test.go | 22 +++++++++++++++++++ 5 files changed, 124 insertions(+), 13 deletions(-) diff --git a/cmd/mavend/clarify.go b/cmd/mavend/clarify.go index 22266e7..512b3d3 100644 --- a/cmd/mavend/clarify.go +++ b/cmd/mavend/clarify.go @@ -45,6 +45,40 @@ var clarifyQuestions = map[dialogue.Slot]string{ // landed. Feminine self-reference ("поняла"), as everywhere. const clarifyGaveUp = "Прости, я не поняла. Скажи, пожалуйста, по-другому." +// clarifyExpired — his answer came after the TTL, so the parked request is +// already gone. Same tone as clarifyGaveUp, different reason: too much time +// passed, not "I did not understand". Feminine self-reference ("ждала", +// "отпустила"); he is addressed with a plain imperative. +const clarifyExpired = "Прости, я слишком долго ждала ответа и отпустила прошлую просьбу. Если она ещё нужна, скажи заново." + +// clarifyExpiredNotice returns that line when a parked question had just timed +// out, and "" when nothing was parked. Call it right after +// resolveClarifyAnswer: a live question is answered there, an expired one is +// only reported here — the words themselves still go on to be routed fresh. +func (h *reactiveHandler) clarifyExpiredNotice() string { + if h.clarifyStore == nil { + return "" + } + if !h.clarifyStore.TakeExpired(voiceDialogueID, h.now()) { + return "" + } + log.Printf("voice: clarify — parked question expired, telling him and routing the words fresh") + return clarifyExpired +} + +// withNotice glues the expiry notice in front of this turn's reply. One turn +// carries one reply on the wire, so the notice cannot be a message of its own — +// but neither the notice nor the fresh answer may be dropped. +func withNotice(notice, reply string) string { + if notice == "" { + return reply + } + if reply == "" { + return notice + } + return notice + " " + reply +} + // missingFor returns the slots a decision still needs, most important first. // Empty ⇒ there is nothing identifiable to ask about. func missingFor(dec router.Decision) []dialogue.Slot { diff --git a/cmd/mavend/clarify_test.go b/cmd/mavend/clarify_test.go index 31178bc..43880b2 100644 --- a/cmd/mavend/clarify_test.go +++ b/cmd/mavend/clarify_test.go @@ -289,6 +289,36 @@ func TestNoQuestionWhenNothingIsMissing(t *testing.T) { } } +// 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 !strings.HasPrefix(reply, clarifyExpired) { + t.Fatalf("expired question must be announced first, got %q", reply) + } + if strings.TrimSpace(strings.TrimPrefix(reply, clarifyExpired)) == "" { + 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, "как дела"); strings.Contains(reply, clarifyExpired) { + t.Fatalf("notice repeated on a later turn: %q", reply) + } +} + // TestNoPendingQuestionFallsThrough — with nothing parked, an utterance routes // normally. func TestNoPendingQuestionFallsThrough(t *testing.T) { diff --git a/cmd/mavend/voice.go b/cmd/mavend/voice.go index 2fde3ce..a72281e 100644 --- a/cmd/mavend/voice.go +++ b/cmd/mavend/voice.go @@ -402,9 +402,17 @@ func (h *reactiveHandler) HandlePushToTalk(ctx context.Context, req voice.PushTo return h.reply(ctx, reply, nil) } - // 1b2. clarify answer — if she asked a question last turn, this utterance is - // its answer, not a fresh command. After the confirm check: a y/n gate is - // armed by her own prompt and is the narrower claim on the utterance. + // 1b2. expired clarify — a question was parked but its TTL ran out, so the + // request behind it is gone. Say that out loud (see clarify.go) and carry + // on: these words are still routed as a fresh utterance below, with the + // notice glued in front of whatever the fresh routing answers. Checked + // BEFORE the answer path: reading a parked question drops an expired one. + expiredNotice := h.clarifyExpiredNotice() + + // 1b3. clarify answer — if she asked a live question last turn, this + // utterance is its answer, not a fresh command. After the confirm check: a + // y/n gate is armed by her own prompt and is the narrower claim on the + // utterance. if reply, handled := h.resolveClarifyAnswer(ctx, text); handled { return h.reply(ctx, reply, nil) } @@ -414,7 +422,7 @@ func (h *reactiveHandler) HandlePushToTalk(ctx context.Context, req voice.PushTo // unreliably (it's a command, not a free-form query), so we match it // before routing. Same pattern as the confirm turn above. if reply, handled := h.resolveQuietToggle(ctx, text); handled { - return h.reply(ctx, reply, nil) + return h.reply(ctx, withNotice(expiredNotice, reply), nil) } // 2. router — classify the utterance. @@ -423,10 +431,10 @@ func (h *reactiveHandler) HandlePushToTalk(ctx context.Context, req voice.PushTo // ErrNoIntents ⇒ classifier unseeded (cold boot). reply with a // "still warming up" rather than a wire error. if errors.Is(err, router.ErrNoIntents) { - return h.reply(ctx, "я ещё не понимаю свободную речь — скоро научусь.", nil) + return h.reply(ctx, withNotice(expiredNotice, "я ещё не понимаю свободную речь — скоро научусь."), nil) } log.Printf("voice: router error: %v", err) - return h.reply(ctx, "не получилось разобрать команду.", nil) + return h.reply(ctx, withNotice(expiredNotice, "не получилось разобрать команду."), nil) } // 2b. dialogue — fill this turn's missing slots from a prior same-intent @@ -447,7 +455,7 @@ func (h *reactiveHandler) HandlePushToTalk(ctx context.Context, req voice.PushTo // stands. if dec.Clarify { if question, asked := h.askClarify(dec); asked { - return h.reply(ctx, question, nil) + return h.reply(ctx, withNotice(expiredNotice, question), nil) } } @@ -463,7 +471,7 @@ func (h *reactiveHandler) HandlePushToTalk(ctx context.Context, req voice.PushTo // 5. tts — synthesise the reply text; return to the voice server which // ships it back on the conn. - return h.reply(ctx, replyText, nil) + return h.reply(ctx, withNotice(expiredNotice, replyText), nil) } // handleText — the core reactive path without stt/tts: confirm check → @@ -478,7 +486,9 @@ func (h *reactiveHandler) handleText(ctx context.Context, text string) string { return reply } - // 1b2. clarify answer — same check as HandlePushToTalk. + // 1b2/1b3. expired clarify then clarify answer — same order and reasons as + // HandlePushToTalk. + expiredNotice := h.clarifyExpiredNotice() if reply, handled := h.resolveClarifyAnswer(ctx, text); handled { return reply } @@ -487,10 +497,10 @@ func (h *reactiveHandler) handleText(ctx context.Context, text string) string { dec, err := h.router.Route(ctx, text, h.now()) if err != nil { if errors.Is(err, router.ErrNoIntents) { - return "я ещё не понимаю свободную речь — скоро научусь." + return withNotice(expiredNotice, "я ещё не понимаю свободную речь — скоро научусь.") } log.Printf("voice: handleText router error: %v", err) - return "не получилось разобрать команду." + return withNotice(expiredNotice, "не получилось разобрать команду.") } log.Printf("voice: route result: intent=%s slots=%+v", dec.Intent, dec.Slots) @@ -507,7 +517,7 @@ func (h *reactiveHandler) handleText(ctx context.Context, text string) string { // 2c. clarify — same as HandlePushToTalk: ask about the one missing thing. if dec.Clarify { if question, asked := h.askClarify(dec); asked { - return question + return withNotice(expiredNotice, question) } } @@ -519,7 +529,7 @@ func (h *reactiveHandler) handleText(ctx context.Context, text string) string { if replyText == "" { replyText = h.replier.Reply(dec) } - return replyText + return withNotice(expiredNotice, replyText) } // applyAction — executes the router's Decision. Intent-by-intent: diff --git a/internal/dialogue/clarify.go b/internal/dialogue/clarify.go index c91b06e..e3a49c3 100644 --- a/internal/dialogue/clarify.go +++ b/internal/dialogue/clarify.go @@ -100,6 +100,21 @@ func (s *ClarifyStore) Get(id string, now time.Time) *PendingQuestion { return q } +// TakeExpired reports whether a question was parked here but its TTL ran out, +// and drops it. Get drops such a question silently, which leaves the user +// thinking his request is still alive — the caller uses this to tell him it is +// gone before treating his words as a fresh utterance. +func (s *ClarifyStore) TakeExpired(id string, now time.Time) bool { + s.mu.Lock() + defer s.mu.Unlock() + q, ok := s.questions[id] + if !ok || !q.IsExpired(now) { + return false + } + delete(s.questions, id) + return true +} + func (s *ClarifyStore) Delete(id string) { s.mu.Lock() delete(s.questions, id) diff --git a/internal/dialogue/clarify_test.go b/internal/dialogue/clarify_test.go index e9bec96..513e2e6 100644 --- a/internal/dialogue/clarify_test.go +++ b/internal/dialogue/clarify_test.go @@ -60,6 +60,28 @@ func TestClarifyStoreGetPutDelete(t *testing.T) { } } +// TestClarifyStoreTakeExpired — TakeExpired reports (and drops) only a question +// whose TTL ran out. +func TestClarifyStoreTakeExpired(t *testing.T) { + s := NewClarifyStore(time.Minute) + if s.TakeExpired("voice", base) { + t.Fatal("nothing parked ⇒ nothing expired") + } + s.Put("voice", &PendingQuestion{Missing: []Slot{SlotTime}, Asked: base, TTL: time.Minute}) + if s.TakeExpired("voice", base.Add(30*time.Second)) { + t.Fatal("a live question must not report as expired") + } + if s.Get("voice", base.Add(30*time.Second)) == nil { + t.Fatal("a live question must survive TakeExpired") + } + if !s.TakeExpired("voice", base.Add(2*time.Minute)) { + t.Fatal("a stale question must report as expired") + } + if s.TakeExpired("voice", base.Add(2*time.Minute)) { + t.Fatal("TakeExpired must drop the question, so the second call is false") + } +} + func TestNewClarifyStoreDefaultTTL(t *testing.T) { s := NewClarifyStore(0) q := &PendingQuestion{Asked: base}