Merge the note recall fix

2026-07-31 13:33:14 +04:00
parent 9a3bcd7c46 98ee701e03
commit 892330eb84
8 changed files with 266 additions and 27 deletions
@@ -0,0 +1,158 @@
+package main
+
+import (
+	"context"
+	"fmt"
+	"math"
+	"testing"
+	"time"
+
+	"github.com/kami/maven/internal/ipc"
+	"github.com/kami/maven/internal/memory"
+	"github.com/kami/maven/internal/phraser"
+	"github.com/kami/maven/internal/router"
+	"github.com/kami/maven/internal/voice"
+)
+
+// fixedEmbedder hands back a vector chosen per text, so a test can say exactly
+// how close each stored memory is to the question. The real embedders make
+// scores that are realistic but not controllable, and this test is about the
+// gate, not about the embedder.
+type fixedEmbedder struct{ vecs map[string][]float32 }
+
+func (f *fixedEmbedder) Dim() int     { return 4 }
+func (f *fixedEmbedder) Close() error { return nil }
+
+func (f *fixedEmbedder) Embed(_ context.Context, text string) ([]float32, error) {
+	v, ok := f.vecs[text]
+	if !ok {
+		return nil, fmt.Errorf("fixedEmbedder: no vector for %q", text)
+	}
+	return v, nil
+}
+
+// scoreVec builds a unit vector whose cosine against the query vector
+// (1,0,0,0) is exactly score.
+func scoreVec(score float64) []float32 {
+	rest := math.Sqrt(1 - score*score)
+	return []float32{float32(score), float32(rest), 0, 0}
+}
+
+// recordingPhraser remembers what the query path handed it to phrase, which is
+// how the test can tell which pass produced the answer.
+type recordingPhraser struct {
+	*phraser.Stub
+	notes []string
+}
+
+func (r *recordingPhraser) PhraseQuery(ctx context.Context, utterance string, notes []string) (string, error) {
+	r.notes = notes
+	return r.Stub.PhraseQuery(ctx, utterance, notes)
+}
+
+// recallCase — one stored memory: its text, how close it is to the question,
+// whether it is a note or a fact, and whether the notes table holds it too.
+type recallCase struct {
+	text  string
+	score float64
+	kind  string
+}
+
+// buildRecallHandler stores the given memories and returns a handler whose
+// query path can be run directly. Notes go into BOTH the notes table and the
+// vector index, which is what the daemon does (voice.go's IntentNote).
+func buildRecallHandler(t *testing.T, question string, mems []recallCase) (*reactiveHandler, *recordingPhraser) {
+	t.Helper()
+	ctx := context.Background()
+	st := newTestStore(t)
+	emb := &fixedEmbedder{vecs: map[string][]float32{question: {1, 0, 0, 0}}}
+	mem := memory.NewInMemoryStore()
+	now := time.Now()
+
+	for i, m := range mems {
+		vec := scoreVec(m.score)
+		emb.vecs[m.text] = vec
+		id := fmt.Sprintf("%s:%d", m.kind, i)
+		if m.kind == "note" {
+			if _, err := st.WriteNote(ctx, now, m.text, vec, "tap:voice"); err != nil {
+				t.Fatalf("WriteNote: %v", err)
+			}
+		}
+		if err := mem.Insert(ctx, id, vec, map[string]string{"text": m.text, "type": m.kind}); err != nil {
+			t.Fatalf("memory insert: %v", err)
+		}
+	}
+
+	phr := &recordingPhraser{Stub: phraser.NewStub()}
+	h := &reactiveHandler{
+		api:             ipc.NewStoreAPI(st),
+		embedder:        emb,
+		replier:         voice.NewStubReplier(),
+		phraser:         phr,
+		now:             func() time.Time { return now },
+		memStore:        mem,
+		dataStore:       st,
+		queryMinScore:   0.55,
+		queryMinMargin:  0.008,
+		weatherProvider: nil,
+	}
+	return h, phr
+}
+
+func askQuery(t *testing.T, h *reactiveHandler, question string) string {
+	t.Helper()
+	return h.applyAction(context.Background(), router.Decision{
+		Intent:    router.IntentQuery,
+		Utterance: question,
+	})
+}
+
+// TestQueryRecallNoteCanWin — the note-recall regression (Vikunja #373). Notes
+// and facts share one vector index, and a note that clearly beats everything
+// else must be the answer. Before the fix the memory pass only ran after the
+// notes-only gate had already rejected the same note at the same score, so only
+// a fact could ever come back from it.
+func TestQueryRecallNoteCanWin(t *testing.T) {
+	const q = "где молоко"
+
+	t.Run("a clearly best note answers", func(t *testing.T) {
+		h, phr := buildRecallHandler(t, q, []recallCase{
+			{text: "молоко стоит в холодильнике", score: 0.90, kind: "note"},
+			{text: "выучил пару аккордов", score: 0.50, kind: "note"},
+		})
+		reply := askQuery(t, h, q)
+		if want := "вот что я нашла: молоко стоит в холодильнике"; reply != want {
+			t.Errorf("reply %q, want %q", reply, want)
+		}
+		// One text, the winning memory's — the answer came from the memory
+		// pass, not from handing the phraser every note in the table.
+		if len(phr.notes) != 1 || phr.notes[0] != "молоко стоит в холодильнике" {
+			t.Errorf("phraser got %q, want just the recalled note", phr.notes)
+		}
+	})
+
+	// The other half of "one gate over everything": a fact that matches better
+	// than the best note now answers, instead of losing to a note that only had
+	// to beat other notes.
+	t.Run("the better-matching fact answers", func(t *testing.T) {
+		h, _ := buildRecallHandler(t, q, []recallCase{
+			{text: "молоко стоит в холодильнике", score: 0.80, kind: "note"},
+			{text: "купил молоко в среду", score: 0.95, kind: "fact"},
+		})
+		if reply := askQuery(t, h, q); reply != "купил молоко в среду" {
+			t.Errorf("reply %q, want the fact read back", reply)
+		}
+	})
+
+	// The gate is untouched: two memories this close mean the embedder cannot
+	// tell them apart, and silence still beats a coin flip.
+	t.Run("no clear best stays silent", func(t *testing.T) {
+		h, _ := buildRecallHandler(t, q, []recallCase{
+			{text: "молоко стоит в холодильнике", score: 0.860, kind: "note"},
+			{text: "молоко закончилось", score: 0.858, kind: "note"},
+		})
+		if reply := askQuery(t, h, q); reply != "не знаю." {
+			t.Errorf("reply %q, want silence", reply)
+		}
+	})
+}
@@ -2,21 +2,24 @@ package main

 import "github.com/kami/maven/internal/memory"

-// bestRecall is the read side of the long-term memory store: the top hit's
-// stored text when it clears the confidence gate. This recalls across BOTH
-// notes and facts (facts aren't in the notes table, so this is the only path
-// that can answer "when did I last …?" from a captured fact). A note hit here
-// is redundant with the notes-RAG path — by design; the two indexes can diverge
-// once the backend is swapped for a persistent/external store. ok=false when
-// the hit fails the confidence gate (see memory.Confident: an absolute floor
-// plus a margin over the runner-up) or carries no text.
-func bestRecall(results []memory.Result, minScore, minMargin float64) (string, bool) {
+// bestRecall is the read side of the long-term memory store: the top hit when
+// it clears the confidence gate. The index holds BOTH notes and facts, and
+// either can win — the caller looks at the returned hit's meta["type"] to see
+// which. Facts aren't in the notes table, so this is the only path that can
+// answer "when did I last …?" from a captured fact.
+//
+// The whole hit is returned, not just its text, because "which memory answered"
+// decides how the answer is said: a note gets phrased in Maven's voice, a fact
+// is read back as stored.
+//
+// ok=false when the hit fails the confidence gate (see memory.Confident: an
+// absolute floor plus a margin over the runner-up) or carries no text.
+func bestRecall(results []memory.Result, minScore, minMargin float64) (memory.Result, bool) {
 	if !memory.Confident(results, minScore, minMargin) {
-		return "", false
+		return memory.Result{}, false
 	}
-	text := results[0].Meta["text"]
-	if text == "" {
-		return "", false
+	if results[0].Meta["text"] == "" {
+		return memory.Result{}, false
 	}
-	return text, true
+	return results[0], true
 }
@@ -39,8 +39,27 @@ func TestBestRecall(t *testing.T) {
 		if !ok {
 			t.Fatal("clearing hit not returned")
 		}
-		if got != "выпил воды в три часа" {
-			t.Errorf("wrong text: %q", got)
+		if got.Meta["text"] != "выпил воды в три часа" {
+			t.Errorf("wrong text: %q", got.Meta["text"])
+		}
+		if got.Meta["type"] != "fact" {
+			t.Errorf("kind lost: %q", got.Meta["type"])
+		}
+	})
+
+	// The index holds notes and facts together, so a note has to be able to win
+	// it — for a long time it could not (Vikunja #373).
+	t.Run("a note can win", func(t *testing.T) {
+		res := []memory.Result{
+			{Score: 0.86, Meta: map[string]string{"text": "молоко в холодильнике", "type": "note"}},
+			{Score: 0.61, Meta: map[string]string{"text": "выпил воды", "type": "fact"}},
+		}
+		got, ok := bestRecall(res, min, margin)
+		if !ok {
+			t.Fatal("clearly-best note not returned")
+		}
+		if got.Meta["type"] != "note" || got.Meta["text"] != "молоко в холодильнике" {
+			t.Errorf("got %v, want the note", got.Meta)
 		}
 	})

@@ -786,11 +786,43 @@ func (h *reactiveHandler) applyAction(ctx context.Context, dec router.Decision)
 			log.Printf("voice: embed query: %v", err)
 			return "не получилось найти ответ."
 		}
+		// Long-term memory first: ONE search over everything Maven remembers
+		// (notes and facts share this index) and ONE confidence gate, so the
+		// memory that is clearly the best match answers — a note just as much
+		// as a fact.
+		//
+		// This used to run only after the notes-only gate below had already
+		// rejected the same note at the same score, which no note could ever
+		// survive a second time: the branch could only return a fact (#373).
+		// Order, not the gate, was the bug — the set of questions Maven answers
+		// is unchanged, only which memory gets to answer them.
+		if h.memStore != nil {
+			if hits, herr := h.memStore.Search(ctx, vec, 3); herr == nil {
+				if hit, ok := bestRecall(hits, h.queryMinScore, h.queryMinMargin); ok {
+					text := hit.Meta["text"]
+					// A note is phrased in Maven's voice; a fact is read back
+					// as it was stored.
+					if hit.Meta["type"] == "note" {
+						if reply, perr := h.phraser.PhraseQuery(ctx, dec.Utterance, []string{text}); perr == nil && reply != "" {
+							return reply
+						}
+					}
+					return text
+				}
+			} else {
+				log.Printf("voice: memory search: %v", herr)
+			}
+		}
+
 		notes, err := h.api.QueryNotes(ctx, vec, 5)
 		if err != nil {
 			log.Printf("voice: query notes: %v", err)
 			return "не получилось найти ответ."
 		}
+		// Notes-only pass, for notes the vector index above does not hold (an
+		// older note written before it existed). Same gate, notes-only
+		// candidates.
+		//
 		// Confidence gate: below it, say "I don't know" rather than read back
 		// the least-unrelated note — a confident wrong recall is worse than a
 		// gap (spec's "not a guesser-of-truth"). Same instinct as the loop's
@@ -802,16 +834,6 @@ func (h *reactiveHandler) applyAction(ctx context.Context, dec router.Decision)
 			noteScores[i] = n.Score
 		}
 		if !memory.ConfidentScores(noteScores, h.queryMinScore, h.queryMinMargin) {
-			// Long-term memory recall (notes + facts) before general knowledge:
-			// the notes table can't answer fact questions, but the memory store
-			// indexes both. Only runs when notes-RAG already gave up → additive.
-			if h.memStore != nil {
-				if hits, herr := h.memStore.Search(ctx, vec, 3); herr == nil {
-					if text, ok := bestRecall(hits, h.queryMinScore, h.queryMinMargin); ok {
-						return text
-					}
-				}
-			}
 			// Try general knowledge from the phraser before giving up
 			reply, err := h.phraser.PhraseQuery(ctx, dec.Utterance, nil)
 			if err != nil || reply == "" {