Keep no secret-shaped question, and hold the two-question cap across nodes (issue 589, review 105)

A question was kept on the bus before the router judged its words, so a token in
it reached a bucket every node reads: every word, the options' too, is now held
to the secret shapes and invisible characters before anything is kept, and a
refused question keeps none of its words. The cap was check-then-create and
failed open: each open question now holds one of two slot keys taken by create
or compare-and-set, refusing when a slot cannot be read. A question the router
never recorded frees its slot after two minutes instead of at expiry. by is
required; wording matches ADR 0332.

modules/claude-code/cmd/claude-code/questions.go

@@ -27,6 +27,7 @@ import (
 	"sync"
 	"time"
 
+	stdio "git.novox.be/novox/mesh-sdk/go"
 	"git.novox.be/novox/mesh-sdk/go/asks"
 )
 
@@ -37,9 +38,25 @@ const QuestionsState = "questions"
 const QuestionPrefix = "q-"
 
 // MostQuestions is how many questions may wait at once, mesh-wide: the router allows an asker three open asks
-// (asks.MostOpen), and one is kept for a proposed instruction section (ADR 0274), so a proposal always finds room.
+// (asks.MostOpen), and questions alone never fill them, so one is left for a proposed instruction section (ADR 0274)
+// while two questions are open. Each open question holds one slot key in the `questions` state (slotKey), taken by
+// create or by compare-and-set, so two nodes asking at once cannot both take the last one.
 const MostQuestions = asks.MostOpen - 1
 
+// RouterHoldsWithin is how long after a question is asked the router has recorded it: it records an ask before it
+// takes its work. A question it holds no record of after that was refused (and the refusal not heard here) or never
+// taken: it no longer holds a slot, and reads as ended unless a record of it appears after all.
+var RouterHoldsWithin = 2 * time.Minute
+
+// slotKey is the key of one of the MostQuestions slots: not a question's key, so no question is ever read from it.
+func slotKey(n int) string { return fmt.Sprintf("slot-%d", n) }
+
+// slot is what a slot key holds: the question holding it.
+type slot struct {
+	Question string    `json:"question"`
+	At       time.Time `json:"at"`
+}
+
 // The bounds of a question's life (ADR 0332 §2): at least five minutes, at most a day less ten minutes (the
 // router's bound for an approval, with the margin a proposal keeps), an hour when none is given.
 const (
@@ -71,6 +88,35 @@ type QuestionOption struct {
 	Recommended bool   `json:"recommended,omitempty"`
 }
 
+// QuestionStore is the `questions` state: a value with its revision, created only when absent, updated only at the
+// revision read.
+type QuestionStore interface {
+	Get(key string) (json.RawMessage, uint64, bool, error)
+	Create(key string, value any) error
+	Update(key string, value any, revision uint64) error
+	Put(key string, value any) error
+}
+
+// keptQuestions is the `questions` state on the bus.
+type keptQuestions struct{ s stdio.KeptState }
+
+func (k keptQuestions) Get(key string) (json.RawMessage, uint64, bool, error) {
+	e, err := k.s.Get(key)
+	if err != nil || e == nil {
+		return nil, 0, false, err
+	}
+	return e.Value, e.Revision, true, nil
+}
+func (k keptQuestions) Create(key string, value any) error {
+	_, err := k.s.Create(key, value)
+	return err
+}
+func (k keptQuestions) Update(key string, value any, revision uint64) error {
+	_, err := k.s.Update(key, value, revision)
+	return err
+}
+func (k keptQuestions) Put(key string, value any) error { _, err := k.s.Put(key, value); return err }
+
 // Question is one question, as the `questions` state keeps it. Every field the ask is composed from is here, so the
 // answer is held to exactly the question asked.
 type Question struct {
@@ -205,11 +251,8 @@ func QuestionProblem(q Question) string {
 	if life := q.Expires.Sub(q.At); life < QuestionShortest || life > QuestionLongest {
 		add("it lasts %s; a question lasts %s to %s", life.Round(time.Minute), QuestionShortest, QuestionLongest)
 	}
-	for _, text := range []string{q.Headline, q.Context, q.Why, q.OnExpiry} {
-		if what := invisible(text); what != "" {
-			add("its words hold %s, which a phone does not show", what)
-			break
-		}
+	if problem := questionWordsProblem(q); problem != "" {
+		add("%s", problem)
 	}
 	if len(problems) == 0 {
 		return ""
@@ -217,6 +260,30 @@ func QuestionProblem(q Question) string {
 	return strings.Join(problems, "; ")
 }
 
+// questionWordsProblem says why a question's words may not be kept at all, or "": anything shaped like a secret, or a
+// character a phone does not show, in any word the operator is shown — the headline, the context, why, what happens
+// on expiry, and each option's label and meaning. Judged before anything is kept: the `questions` state is read by
+// every node, and the router judges the content rule only after the question was kept.
+func questionWordsProblem(q Question) string {
+	fields := []struct{ name, text string }{{"the headline", q.Headline}, {"the context", q.Context}, {"why", q.Why},
+		{"what happens on expiry", q.OnExpiry}}
+	for i, o := range q.Options {
+		fields = append(fields, struct{ name, text string }{fmt.Sprintf("option %d's label", i+1), o.Label},
+			struct{ name, text string }{fmt.Sprintf("what option %d means", i+1), o.Means})
+	}
+	for _, f := range fields {
+		for _, re := range secretShaped {
+			if re.MatchString(f.text) {
+				return f.name + " holds something shaped like a secret, which is never kept or shown on a phone"
+			}
+		}
+		if what := invisible(f.text); what != "" {
+			return f.name + " holds " + what + ", which a phone does not show"
+		}
+	}
+	return ""
+}
+
 func newQuestionID() string {
 	var b [8]byte
 	_, _ = rand.Read(b[:])
@@ -234,9 +301,9 @@ type QuestionInput struct {
 	Expires  time.Duration
 }
 
-// AskQuestion keeps a question and asks the operator. It answers at once with the question's key; the answer is read
-// with ReadAnswer.
-func AskQuestion(p Paths, in QuestionInput, store ProposalStore, bus AskBus, now time.Time) (map[string]any, error) {
+// AskQuestion keeps a question and asks the operator. It answers within seconds, once the router has said whether it
+// took the question, with the question's key; the answer is read with ReadAnswer.
+func AskQuestion(p Paths, in QuestionInput, store QuestionStore, bus AskBus, now time.Time) (map[string]any, error) {
 	refuse := func(why string) (map[string]any, error) { return nil, errors.New(why + ". Nothing was asked") }
 	by := strings.TrimSpace(in.By)
 	if who, node, found := strings.Cut(by, "@"); found {
@@ -246,7 +313,7 @@ func AskQuestion(p Paths, in QuestionInput, store ProposalStore, bus AskBus, now
 		by = who
 	}
 	if by == "" {
-		by = "unnamed"
+		return refuse("it does not say who asks: give by, your session's name as who@node, such as fix-589@g14")
 	}
 	life := in.Expires
 	if life == 0 {
@@ -262,21 +329,25 @@ func AskQuestion(p Paths, in QuestionInput, store ProposalStore, bus AskBus, now
 	if problem := QuestionProblem(q); problem != "" {
 		return refuse(problem)
 	}
-	if open := openQuestions(store, bus, now); len(open) >= MostQuestions {
-		return refuse(fmt.Sprintf("%d questions already wait for the operator's answer (%s), and sessions hold at most %d at "+
-			"once so a proposed instruction section always finds room; read them with claude_code_answer, or wait for "+
-			"one to end", len(open), strings.Join(open, ", "), MostQuestions))
-	}
 	ask := q.Ask()
 	if err := ask.Check(now); err != nil {
 		return refuse(err.Error())
 	}
+	// The question is kept before its slot is taken, so a slot always names a question another node can judge.
 	if err := store.Create(q.ID, q); err != nil {
 		return nil, fmt.Errorf("the question could not be kept, so the operator was not asked: %w", err)
 	}
+	if open, err := takeSlot(store, bus, q, now); err != nil {
+		_ = store.Put(q.ID, tombstone(q, "not asked: "+err.Error()))
+		return refuse(err.Error())
+	} else if open != nil {
+		_ = store.Put(q.ID, tombstone(q, "not asked: two questions were open"))
+		return refuse(fmt.Sprintf("%d questions already wait for the operator's answer (%s), and sessions hold at most %d "+
+			"at once, so one of the router's three open asks is left for a proposed instruction section; read them with "+
+			"claude_code_answer, or wait for one to end", len(open), strings.Join(open, ", "), MostQuestions))
+	}
 	if err := bus.Publish(asks.AskSubject(Module), ask, ask.ID); err != nil {
-		q.Asked = "not asked: " + err.Error()
-		_ = store.Put(q.ID, q)
+		_ = store.Put(q.ID, tombstone(q, "not asked: "+err.Error()))
 		return nil, fmt.Errorf("the operator could not be asked: %v", err)
 	}
 	say("question %s from the session %s asked of the operator", q.ID, q.By)
@@ -310,43 +381,99 @@ func labelsOf(q Question) string {
 	return strings.Join(out, ", ")
 }
 
-// openQuestions are the keys of the questions still waiting: asked, not ended, not expired.
-func openQuestions(store ProposalStore, bus AskBus, now time.Time) []string {
-	keys, err := store.Keys()
+// questionOpen says whether a question still waits: kept, asked, not ended, not expired, and held by the router or
+// asked less than RouterHoldsWithin ago. An unreadable record is open: a slot is never freed on a guess.
+func questionOpen(store QuestionStore, bus AskBus, id string, now time.Time) (bool, error) {
+	q, ok, err := readQuestionErr(store, id)
 	if err != nil {
-		return nil
+		return true, err
 	}
+	if !ok || q.Asked != "" || q.Ended != "" || !now.Before(q.Expires) {
+		return false, nil
+	}
+	entry, err := bus.Record(q.ID)
+	if err != nil {
+		return true, err
+	}
+	if entry == nil {
+		return now.Sub(q.At) < RouterHoldsWithin, nil
+	}
+	var rec routerRecord
+	if json.Unmarshal(entry.Value, &rec) != nil {
+		return true, nil
+	}
+	return rec.State == "" || rec.State == "open", nil
+}
+
+// takeSlot takes one of the MostQuestions slots for q: a slot no key holds, by create, or one whose question no longer
+// waits, by compare-and-set at the revision read. Of two nodes taking the same slot only one wins; the other tries
+// the next. It answers the open questions when every slot is held, and an error — refusing, never guessing — when a
+// slot or its question cannot be read.
+func takeSlot(store QuestionStore, bus AskBus, q Question, now time.Time) ([]string, error) {
+	mine := slot{Question: q.ID, At: q.At}
 	var open []string
-	for _, k := range keys {
-		q, ok := readQuestion(store, k)
-		if !ok || q.Asked != "" || q.Ended != "" || !now.Before(q.Expires) {
-			continue
+	for n := 1; n <= MostQuestions; n++ {
+		key := slotKey(n)
+		raw, rev, found, err := store.Get(key)
+		if err != nil {
+			return nil, fmt.Errorf("the open questions could not be counted (%v), so none more is asked", err)
 		}
-		if entry, err := bus.Record(q.ID); err == nil && entry != nil {
-			var rec routerRecord
-			if json.Unmarshal(entry.Value, &rec) == nil && rec.State != "" && rec.State != "open" {
+		if !found {
+			if err := store.Create(key, mine); err == nil {
+				return nil, nil
+			} else if !errors.Is(err, stdio.ErrChanged) {
+				return nil, fmt.Errorf("the open questions could not be counted (%v), so none more is asked", err)
+			}
+			if raw, rev, found, err = store.Get(key); err != nil || !found {
+				return nil, fmt.Errorf("the open questions could not be counted, so none more is asked")
+			}
+		}
+		var held slot
+		if json.Unmarshal(raw, &held) == nil && held.Question != "" {
+			waits, err := questionOpen(store, bus, held.Question, now)
+			if err != nil {
+				return nil, fmt.Errorf("the open questions could not be counted (%v), so none more is asked", err)
+			}
+			if waits {
+				open = append(open, held.Question)
 				continue
 			}
 		}
-		open = append(open, q.ID)
+		if err := store.Update(key, mine, rev); err == nil {
+			return nil, nil
+		} else if !errors.Is(err, stdio.ErrChanged) {
+			return nil, fmt.Errorf("the open questions could not be counted (%v), so none more is asked", err)
+		}
+		open = append(open, "one taken just now")
 	}
 	sort.Strings(open)
-	return open
+	return open, nil
 }
 
-func readQuestion(store ProposalStore, key string) (Question, bool) {
+// tombstone is what is kept of a question that was not asked or that the router refused: its key, who asked, when, and
+// why it ended — none of its words, which the router judged unfit to leave the mesh or never saw.
+func tombstone(q Question, ended string) Question {
+	return Question{ID: q.ID, By: q.By, From: q.From, At: q.At, Expires: q.Expires, Ended: ended}
+}
+
+func readQuestion(store QuestionStore, key string) (Question, bool) {
+	q, ok, _ := readQuestionErr(store, key)
+	return q, ok
+}
+
+func readQuestionErr(store QuestionStore, key string) (Question, bool, error) {
 	if !strings.HasPrefix(key, QuestionPrefix) {
-		return Question{}, false
+		return Question{}, false, nil
 	}
-	raw, found, err := store.Get(key)
+	raw, _, found, err := store.Get(key)
 	if err != nil || !found {
-		return Question{}, false
+		return Question{}, false, err
 	}
 	var q Question
 	if json.Unmarshal(raw, &q) != nil || q.ID != key {
-		return Question{}, false
+		return Question{}, false, nil
 	}
-	return q, true
+	return q, true, nil
 }
 
 // The router's word that a question ended without a choice, as heard on the decided subject.
@@ -369,9 +496,10 @@ func forgetQuestionEnd(id string) {
 }
 
 // HeardQuestionEnd takes the router's word that a question ended without a choice — refused, expired, cancelled or
-// replaced — and keeps it on the question, because a refused ask leaves no record of the router's. A choice is
-// never taken from this word: only from the router's record, held to the question (ReadAnswer).
-func HeardQuestionEnd(store ProposalStore, w asks.Warrant) {
+// replaced — and keeps it on the question, because a refused ask leaves no record of the router's. A refused question
+// keeps none of its words (tombstone). A choice is never taken from this word: only from the router's record, held to
+// the question (ReadAnswer).
+func HeardQuestionEnd(store QuestionStore, w asks.Warrant) {
 	if w.Asker != Module || w.Outcome == asks.OutcomeChosen || !strings.HasPrefix(w.Ask, QuestionPrefix) {
 		return
 	}
@@ -380,9 +508,14 @@ func HeardQuestionEnd(store ProposalStore, w asks.Warrant) {
 		return
 	}
 	if q.Ended == "" {
-		q.Ended = string(w.Outcome)
+		ended := string(w.Outcome)
 		if w.Words != "" {
-			q.Ended += ": " + w.Words
+			ended += ": " + w.Words
+		}
+		if w.Outcome == asks.OutcomeRefused {
+			q = tombstone(q, ended)
+		} else {
+			q.Ended = ended
 		}
 		_ = store.Put(q.ID, q)
 	}
@@ -401,7 +534,7 @@ const (
 )
 
 // ReadAnswer is where a question stands: waiting, chosen (and what), ended without a choice, or not proven.
-func ReadAnswer(store ProposalStore, bus AskBus, key string, now time.Time) (map[string]any, error) {
+func ReadAnswer(store QuestionStore, bus AskBus, key string, now time.Time) (map[string]any, error) {
 	q, ok := readQuestion(store, strings.TrimSpace(key))
 	if !ok {
 		return nil, fmt.Errorf("no question %q is kept: the key is the one claude_code_ask gave", key)
@@ -426,8 +559,9 @@ func ReadAnswer(store ProposalStore, bus AskBus, key string, now time.Time) (map
 	switch {
 	case entry == nil && q.Ended != "":
 		out["state"], out["words"] = AnswerEnded, q.Ended
-	case entry == nil && now.After(q.Expires.Add(time.Hour)):
-		out["state"], out["words"] = AnswerEnded, "the router keeps no record of it: refused before it was taken, or forgotten"
+	case entry == nil && now.Sub(q.At) >= RouterHoldsWithin:
+		out["state"], out["words"] = AnswerEnded, "the router holds no record of it: refused (its refusal was not heard "+
+			"here) or never taken; nothing waits for the operator's answer"
 	case entry == nil, rec.State == "open", rec.Warrant == nil:
 		out["state"] = AnswerWaiting
 		if q.Ended != "" {
@@ -472,7 +606,7 @@ func ReadAnswer(store ProposalStore, bus AskBus, key string, now time.Time) (map
 }
 
 // WaitForAnswer reads the answer, and while it is waiting reads it again until wait has passed.
-func WaitForAnswer(store ProposalStore, bus AskBus, key string, wait time.Duration, now func() time.Time) (map[string]any, error) {
+func WaitForAnswer(store QuestionStore, bus AskBus, key string, wait time.Duration, now func() time.Time) (map[string]any, error) {
 	if wait > AnswerWaitsAtMost {
 		wait = AnswerWaitsAtMost
 	}