diff --git a/internal/dialogue/clarify.go b/internal/dialogue/clarify.go index 98850c2..4f5408a 100644 --- a/internal/dialogue/clarify.go +++ b/internal/dialogue/clarify.go @@ -38,21 +38,34 @@ type PendingQuestion struct { MaxAttempts int } -// maxAttempts is MaxAttempts with the default filled in. -func (q *PendingQuestion) maxAttempts() int { - if q.MaxAttempts <= 0 { - return DefaultMaxAttempts +// Action reads the parked question as the typed action it is assembling +// (pending.go). Derived rather than stored: the question's fields stay the one +// copy of the truth, so a caller that fills them the old way cannot end up with +// a capability that disagrees with the intent. +func (q *PendingQuestion) Action() PendingAction { + return PendingAction{ + Capability: CapabilityFor(q.Intent), + Slots: q.Slots, + Missing: q.Missing, + Utterance: q.Utterance, + Asked: q.Asked, + TTL: q.TTL, + Attempts: q.Attempts, + MaxAttempts: q.MaxAttempts, } - return q.MaxAttempts } +// IsExpired and CanAsk answer through the action, so there is exactly one copy +// of the TTL and attempt-cap rules and the widening cannot drift from them. func (q *PendingQuestion) IsExpired(now time.Time) bool { - return now.After(q.Asked.Add(q.TTL)) + a := q.Action() + return a.IsExpired(now) } // CanAsk reports whether Maven may ask another question about this request. func (q *PendingQuestion) CanAsk() bool { - return q.Attempts < q.maxAttempts() + a := q.Action() + return a.CanAsk() } // ClarifyStore holds the parked questions. Same shape and locking as @@ -66,11 +79,26 @@ func (q *PendingQuestion) CanAsk() bool { // next words route fresh, which is the right answer with or without a notice. // Do not give this store a persister without re-arguing that. type ClarifyStore struct { - mu sync.RWMutex - questions map[string]*PendingQuestion + mu sync.RWMutex + // stacks — one stack of parked questions per dialogue id, newest last. It + // was a single question per id until V-559; a side query has to be able to + // suspend the active flow and find it still there afterwards (V-561 does + // the suspending, this only holds the room for it). + stacks map[string][]*PendingQuestion defaultTTL time.Duration } +// MaxStackDepth — how many parked questions one dialogue id may hold. +// +// Two, not three. One is the flow he is in, one is the thing he interrupted it +// with, and out loud he does not nest deeper than that: a side query inside a +// side query is a shape typed conversation has and spoken conversation does +// not. The bound is also a promise — every level she keeps is a level she must +// be able to SPEAK when it dies (clarifyGaveUp, clarifyExpiredVariants), and +// two lines of "and the other thing I dropped" is already the limit of what a +// reply can carry. +const MaxStackDepth = 2 + func NewClarifyStore(defaultTTL time.Duration) *ClarifyStore { if defaultTTL <= 0 { // Short, like confirmTTL in voice.go: a clarifying question is a @@ -78,27 +106,58 @@ func NewClarifyStore(defaultTTL time.Duration) *ClarifyStore { defaultTTL = 90 * time.Second } return &ClarifyStore{ - questions: make(map[string]*PendingQuestion), + stacks: make(map[string][]*PendingQuestion), defaultTTL: defaultTTL, } } -// Put parks a question. Called on a clarify decision (cmd/mavend/clarify.go). +// Put parks a question, replacing the one on top. Called on a clarify decision +// (cmd/mavend/clarify.go), and it is still what the daemon uses: re-asking the +// same request is a new question about the SAME action, so it overwrites rather +// than growing the stack. Push is the deeper one, and nothing calls it yet. func (s *ClarifyStore) Put(id string, q *PendingQuestion) { - if q.TTL <= 0 { - q.TTL = s.defaultTTL - } + s.fillTTL(q) s.mu.Lock() - s.questions[id] = q - s.mu.Unlock() + defer s.mu.Unlock() + stack := s.stacks[id] + if len(stack) == 0 { + s.stacks[id] = []*PendingQuestion{q} + return + } + stack[len(stack)-1] = q } -// Get returns the live parked question, or nil when there is none. -func (s *ClarifyStore) Get(id string, now time.Time) *PendingQuestion { +// Push suspends whatever is parked and puts q on top. The returned question is +// one the depth bound forced out of the bottom of the stack, and the caller MUST +// tell him about it — a parked request that dies without a word leaves him +// thinking it landed, which is the whole reason clarifyGaveUp exists. nil is the +// ordinary case. +func (s *ClarifyStore) Push(id string, q *PendingQuestion) *PendingQuestion { + s.fillTTL(q) + s.mu.Lock() + defer s.mu.Unlock() + stack := append(s.stacks[id], q) + var dropped *PendingQuestion + if len(stack) > MaxStackDepth { + dropped = stack[0] + stack = stack[1:] + } + s.stacks[id] = stack + return dropped +} + +// Peek returns the live question on top, or nil when there is none. Expired +// entries below it are left alone: TakeExpired is what reports those, and +// dropping one here would be the silent death this store is careful about. +func (s *ClarifyStore) Peek(id string, now time.Time) *PendingQuestion { s.mu.RLock() - q, ok := s.questions[id] + stack := s.stacks[id] + var q *PendingQuestion + if len(stack) > 0 { + q = stack[len(stack)-1] + } s.mu.RUnlock() - if !ok { + if q == nil { return nil } if q.IsExpired(now) { @@ -108,27 +167,81 @@ func (s *ClarifyStore) Get(id string, now time.Time) *PendingQuestion { return q } +// Get is Peek under the name every caller already uses. Kept because a clarify +// answer is always about the top of the stack, so the two are the same call. +func (s *ClarifyStore) Get(id string, now time.Time) *PendingQuestion { + return s.Peek(id, now) +} + +// Pop takes the live question off the top and returns it, so the flow beneath +// becomes current again. nil when the top is empty or expired — an expired top +// is dropped along with the rest of the stack, exactly as Peek does, because the +// clock that killed it has been running for everything underneath too. +func (s *ClarifyStore) Pop(id string, now time.Time) *PendingQuestion { + s.mu.Lock() + stack := s.stacks[id] + if len(stack) == 0 { + s.mu.Unlock() + return nil + } + q := stack[len(stack)-1] + if q.IsExpired(now) { + delete(s.stacks, id) + s.mu.Unlock() + return nil + } + if len(stack) == 1 { + delete(s.stacks, id) + } else { + s.stacks[id] = stack[:len(stack)-1] + } + s.mu.Unlock() + return q +} + +// Depth — how many questions are parked for this id, expired ones included. +// Diagnostic; the arbiter in V-560 reads it to know it is inside a flow. +func (s *ClarifyStore) Depth(id string) int { + s.mu.RLock() + defer s.mu.RUnlock() + return len(s.stacks[id]) +} + // 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. +// +// It looks at the top only, and drops the whole stack when that one is dead: one +// notice is what a reply can carry, and anything parked under a question that +// timed out has been waiting at least as long. 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) { + stack := s.stacks[id] + if len(stack) == 0 || !stack[len(stack)-1].IsExpired(now) { return false } - delete(s.questions, id) + delete(s.stacks, id) return true } +// Delete drops every question parked for this id. The old single-slot Delete +// under the old name: at depth one the two are the same, and a caller that means +// "this exchange is over" means all of it. func (s *ClarifyStore) Delete(id string) { s.mu.Lock() - delete(s.questions, id) + delete(s.stacks, id) s.mu.Unlock() } +// fillTTL applies the store default to a question parked without one. +func (s *ClarifyStore) fillTTL(q *PendingQuestion) { + if q.TTL <= 0 { + q.TTL = s.defaultTTL + } +} + // Answer merges the slots parsed from the user's answer into the parked ones. // 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 «нет, в пять» diff --git a/internal/dialogue/pending.go b/internal/dialogue/pending.go new file mode 100644 index 0000000..101407e --- /dev/null +++ b/internal/dialogue/pending.go @@ -0,0 +1,101 @@ +package dialogue + +import "time" + +// Capability names the thing being assembled across a clarify exchange — +// "reminder.create", not "reminder". A router intent says what she heard; a +// capability says what she is about to do, and those are not the same word: +// three intents currently reach exactly one capability each, but a fact key +// that turns out to be a Hexis target does not. Named in the ecosystem's +// dotted form because that is what a confirmation binds (cmd/mavend/confirm.go) +// and what Hexis registers. +// +// This package must stay free of internal/router (the cycle rule that makes +// Slots a hand-kept copy), so the mapping from an intent lives here and reads +// off dialogue.Intent only. +type Capability string + +const ( + CapReminderCreate Capability = "reminder.create" + CapFactWrite Capability = "fact.write" + CapNoteWrite Capability = "note.write" + CapActRun Capability = "act.run" + CapQueryAnswer Capability = "query.answer" + CapChatReply Capability = "chat.reply" + CapSystemControl Capability = "system.control" +) + +// intentCapability — the one place an intent becomes a capability. Every intent +// is listed, including the four that are never worth a clarifying question, so a +// parked action always knows what it is even when nothing asks it. +var intentCapability = map[Intent]Capability{ + IntentReminder: CapReminderCreate, + IntentFact: CapFactWrite, + IntentNote: CapNoteWrite, + IntentAct: CapActRun, + IntentQuery: CapQueryAnswer, + IntentChat: CapChatReply, + IntentSystem: CapSystemControl, +} + +// CapabilityFor maps a router intent (already narrowed to dialogue.Intent by +// the caller) to the capability being assembled. "" for an intent she does not +// recognise — an unknown intent must not silently become a real capability. +func CapabilityFor(in Intent) Capability { + return intentCapability[in] +} + +// PendingAction is the action Maven is assembling, as an object rather than as +// conversational history: which capability, the slots it already has, the slots +// it is still missing, when she asked, how many questions that has cost and how +// long the answer stays welcome. +// +// It exists because the resolver used to have to infer all of that from a +// parked question plus the previous turn (Vikunja #558): "is this his answer or +// a new request" is answerable against an object and guessy against a +// transcript. PendingQuestion carries one of these and keeps its own flat +// fields, so this is a widening — nothing reads the capability yet. +type PendingAction struct { + Capability Capability + Slots Slots // what is filled so far + Missing []Slot // what she is waiting for, in the order to ask about + Utterance string // his original raw words, as the action's provenance + Asked time.Time + TTL time.Duration + Attempts int // questions already asked about this action + // MaxAttempts caps Attempts. 0 ⇒ DefaultMaxAttempts. + MaxAttempts int +} + +// maxAttempts is MaxAttempts with the default filled in. +func (a *PendingAction) maxAttempts() int { + if a.MaxAttempts <= 0 { + return DefaultMaxAttempts + } + return a.MaxAttempts +} + +// IsExpired — the answer came too late for this action to still be his answer. +func (a *PendingAction) IsExpired(now time.Time) bool { + return now.After(a.Asked.Add(a.TTL)) +} + +// CanAsk reports whether she may ask another question about this action. +func (a *PendingAction) CanAsk() bool { + return a.Attempts < a.maxAttempts() +} + +// Gaps lists the slots this action asked for and still does not have. Computed +// from the slots rather than trusted from Missing, because Missing is what she +// asked about and the slots are what she got — an answer can fill a gap she +// never asked about, and a re-park must not ask again for something now filled. +func (a *PendingAction) Gaps() []Slot { + return StillMissing(a.Missing, a.Slots) +} + +// Complete reports whether every slot this action was waiting for is filled, so +// it can run. Note that this is completeness against what she ASKED, not +// against the capability's whole schema — validating that is V-562. +func (a *PendingAction) Complete() bool { + return len(a.Gaps()) == 0 +} diff --git a/internal/dialogue/pending_test.go b/internal/dialogue/pending_test.go new file mode 100644 index 0000000..490e84e --- /dev/null +++ b/internal/dialogue/pending_test.go @@ -0,0 +1,117 @@ +package dialogue + +import ( + "testing" + "time" +) + +var pendingBase = time.Date(2026, 8, 6, 9, 0, 0, 0, time.UTC) + +func TestCapabilityForCoversEveryIntent(t *testing.T) { + for _, in := range []Intent{ + IntentAct, IntentReminder, IntentFact, IntentNote, + IntentQuery, IntentChat, IntentSystem, + } { + if CapabilityFor(in) == "" { + t.Errorf("intent %q maps to no capability", in) + } + } + if got := CapabilityFor(Intent("nonsense")); got != "" { + t.Errorf("unknown intent became capability %q, want empty", got) + } +} + +// A parked question must read as the action it is assembling, without the +// caller having to name the capability twice. +func TestPendingQuestionActionDerivesCapability(t *testing.T) { + q := &PendingQuestion{ + Intent: IntentReminder, + Slots: Slots{Text: "позвонить маме"}, + Missing: []Slot{SlotTime}, + Utterance: "напомни позвонить маме", + Asked: pendingBase, + TTL: time.Minute, + Attempts: 1, + MaxAttempts: 2, + } + a := q.Action() + if a.Capability != CapReminderCreate { + t.Errorf("capability = %q, want %q", a.Capability, CapReminderCreate) + } + if a.Utterance != q.Utterance || a.Attempts != 1 || a.MaxAttempts != 2 || !a.Asked.Equal(pendingBase) { + t.Errorf("action did not carry the question's fields: %+v", a) + } +} + +func TestPendingActionGaps(t *testing.T) { + for _, tc := range []struct { + name string + action PendingAction + want []Slot + complete bool + }{ + { + name: "time still missing", + action: PendingAction{Missing: []Slot{SlotTime}, Slots: Slots{Text: "позвонить маме"}}, + want: []Slot{SlotTime}, + complete: false, + }, + { + name: "asked slot now filled", + action: PendingAction{Missing: []Slot{SlotTime}, Slots: Slots{Time: pendingBase, HasTime: true}}, + want: nil, + complete: true, + }, + { + name: "two gaps reported in ask order", + action: PendingAction{Missing: []Slot{SlotText, SlotTime}}, + want: []Slot{SlotText, SlotTime}, + complete: false, + }, + { + name: "nothing asked is complete", + action: PendingAction{}, + want: nil, + complete: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := tc.action.Gaps() + if len(got) != len(tc.want) { + t.Fatalf("gaps = %v, want %v", got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("gaps = %v, want %v", got, tc.want) + } + } + if tc.action.Complete() != tc.complete { + t.Errorf("Complete() = %v, want %v", tc.action.Complete(), tc.complete) + } + }) + } +} + +// The typed action must answer the TTL and attempt-cap questions the same way +// the parked question always did — this is a widening, not new behaviour. +func TestPendingActionTTLAndAttempts(t *testing.T) { + a := PendingAction{Asked: pendingBase, TTL: time.Minute} + if a.IsExpired(pendingBase.Add(30 * time.Second)) { + t.Error("expired inside the TTL") + } + if !a.IsExpired(pendingBase.Add(2 * time.Minute)) { + t.Error("not expired past the TTL") + } + a.Attempts = DefaultMaxAttempts - 1 + if !a.CanAsk() { + t.Error("cannot ask with an attempt left") + } + a.Attempts = DefaultMaxAttempts + if a.CanAsk() { + t.Error("asked past the default cap") + } + a = PendingAction{Asked: pendingBase, TTL: time.Minute, MaxAttempts: 1, Attempts: 1} + if a.CanAsk() { + t.Error("asked past an explicit cap of 1") + } +} diff --git a/internal/dialogue/stack_test.go b/internal/dialogue/stack_test.go new file mode 100644 index 0000000..8cd9156 --- /dev/null +++ b/internal/dialogue/stack_test.go @@ -0,0 +1,155 @@ +package dialogue + +import ( + "testing" + "time" +) + +func parked(text string, asked time.Time) *PendingQuestion { + return &PendingQuestion{ + Intent: IntentReminder, + Missing: []Slot{SlotTime}, + Utterance: text, + Asked: asked, + TTL: time.Minute, + } +} + +func TestStackPushPeekPop(t *testing.T) { + s := NewClarifyStore(time.Minute) + if dropped := s.Push("voice", parked("напомни позвонить маме", pendingBase)); dropped != nil { + t.Fatalf("first push dropped %q", dropped.Utterance) + } + if dropped := s.Push("voice", parked("погода в риме", pendingBase)); dropped != nil { + t.Fatalf("second push dropped %q", dropped.Utterance) + } + if got := s.Depth("voice"); got != 2 { + t.Fatalf("depth = %d, want 2", got) + } + if got := s.Peek("voice", pendingBase); got == nil || got.Utterance != "погода в риме" { + t.Fatalf("peek = %+v, want the newest", got) + } + // Peek must not consume: two peeks are the same question. + if got := s.Peek("voice", pendingBase); got == nil || got.Utterance != "погода в риме" { + t.Fatalf("second peek = %+v, want the newest still", got) + } + got := s.Pop("voice", pendingBase) + if got == nil || got.Utterance != "погода в риме" { + t.Fatalf("pop = %+v, want the newest", got) + } + // The flow underneath survived the one on top of it. + if got := s.Peek("voice", pendingBase); got == nil || got.Utterance != "напомни позвонить маме" { + t.Fatalf("after pop, peek = %+v, want the suspended flow", got) + } + if got := s.Pop("voice", pendingBase); got == nil { + t.Fatal("pop of the last entry returned nil") + } + if s.Peek("voice", pendingBase) != nil || s.Depth("voice") != 0 { + t.Error("stack not empty after popping everything") + } + if s.Pop("voice", pendingBase) != nil { + t.Error("pop of an empty stack returned something") + } +} + +// A popped entry is gone: it must not come back on the next peek. +func TestStackPoppedEntryIsGone(t *testing.T) { + s := NewClarifyStore(time.Minute) + s.Push("voice", parked("напомни", pendingBase)) + s.Pop("voice", pendingBase) + if got := s.Peek("voice", pendingBase); got != nil { + t.Errorf("peek after pop = %+v, want nil", got) + } +} + +// Past MaxStackDepth the oldest entry comes back to the caller instead of +// vanishing — it is the caller's job to say it was dropped. +func TestStackDepthBoundReturnsTheDroppedEntry(t *testing.T) { + s := NewClarifyStore(time.Minute) + for i := 0; i < MaxStackDepth; i++ { + if dropped := s.Push("voice", parked("first", pendingBase)); dropped != nil { + t.Fatalf("push %d dropped early", i) + } + } + dropped := s.Push("voice", parked("newest", pendingBase)) + if dropped == nil { + t.Fatal("push past the bound dropped an entry silently") + } + if dropped.Utterance != "first" { + t.Errorf("dropped %q, want the oldest", dropped.Utterance) + } + if got := s.Depth("voice"); got != MaxStackDepth { + t.Errorf("depth = %d, want %d", got, MaxStackDepth) + } + if got := s.Peek("voice", pendingBase); got == nil || got.Utterance != "newest" { + t.Errorf("peek = %+v, want the newest", got) + } +} + +// Put still replaces rather than stacks: a re-ask is another question about the +// same action, so the daemon's depth stays one. +func TestPutReplacesTopWithoutGrowing(t *testing.T) { + s := NewClarifyStore(time.Minute) + s.Put("voice", parked("напомни", pendingBase)) + s.Put("voice", parked("напомни ещё раз", pendingBase)) + if got := s.Depth("voice"); got != 1 { + t.Fatalf("depth = %d, want 1", got) + } + if got := s.Peek("voice", pendingBase); got == nil || got.Utterance != "напомни ещё раз" { + t.Fatalf("peek = %+v, want the replacement", got) + } +} + +// An expired top takes the stack with it, and TakeExpired is what reports it — +// the whole exchange timed out, and one notice is what a reply can carry. +func TestStackExpiryDropsTheStackAndIsReported(t *testing.T) { + s := NewClarifyStore(time.Minute) + s.Push("voice", parked("напомни", pendingBase)) + s.Push("voice", parked("погода", pendingBase)) + late := pendingBase.Add(2 * time.Minute) + if s.Peek("voice", late) != nil { + t.Error("peek returned an expired question") + } + if s.Depth("voice") != 0 { + t.Error("expired stack survived a peek") + } + + s.Push("voice", parked("напомни", pendingBase)) + s.Push("voice", parked("погода", pendingBase)) + if !s.TakeExpired("voice", late) { + t.Error("TakeExpired did not report the timed-out exchange") + } + if s.Depth("voice") != 0 { + t.Error("TakeExpired left entries behind") + } + if s.TakeExpired("voice", late) { + t.Error("TakeExpired reported twice") + } + // Pop of an expired top yields nothing rather than a dead action. + s.Push("voice", parked("напомни", pendingBase)) + if s.Pop("voice", late) != nil { + t.Error("pop returned an expired question") + } +} + +// Delete ends the exchange, every level of it. +func TestStackDeleteDropsAll(t *testing.T) { + s := NewClarifyStore(time.Minute) + s.Push("voice", parked("напомни", pendingBase)) + s.Push("voice", parked("погода", pendingBase)) + s.Delete("voice") + if s.Depth("voice") != 0 || s.Peek("voice", pendingBase) != nil { + t.Error("Delete left questions parked") + } +} + +// Stacks are per dialogue id: the mic and the web must not read each other's. +func TestStacksAreIsolatedByID(t *testing.T) { + s := NewClarifyStore(time.Minute) + s.Push("voice", parked("напомни", pendingBase)) + s.Push("web", parked("погода", pendingBase)) + s.Delete("voice") + if got := s.Peek("web", pendingBase); got == nil || got.Utterance != "погода" { + t.Errorf("web stack = %+v, want its own question", got) + } +}