dialogue: the clarify store holds a bounded stack (V-559)

One parked question per dialogue id meant a side query could only destroy
the flow it interrupted. The store now keeps a stack per id, newest last,
with Push, Peek, Pop, Depth and Delete as drop-all. MaxStackDepth is 2:
one flow plus the thing he interrupted it with, because spoken
conversation does not nest deeper, and because every level she keeps is a
level she has to be able to speak when it dies.

Behaviour is unchanged. Put still replaces the top rather than growing the
stack — a re-ask is another question about the same action — and nothing
calls Push yet, so the daemon runs at depth one exactly as before. Get is
Peek under the name the callers already use. An expired top takes the
stack with it and TakeExpired reports it, so no parked action dies without
a word; Push returns the entry the depth bound forced out for the same
reason.

PendingQuestion.IsExpired and CanAsk now answer through PendingAction, so
the TTL and attempt-cap rules have one copy and the widening cannot drift.

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 «нет, в пять»