fix(retrieval): cache the whole result, so a repeat keeps its pincites

pkg/retrieval/cached.go

@@ -11,9 +11,13 @@ import (
 // Cached wraps any Strategy and caches its Select results in an LRU.
 //
 // Cache key = sha256(document_id || query || strategy_name || model_name).
-// The cached value is the []tree.SectionID slice; the wrapping is
-// intentionally transparent — callers interact with the standard Strategy
-// interface and don't know caching is present.
+// The cached value is a copy of the whole Result, so a hit returns what
+// the strategy found: for a page strategy that includes the evidence
+// pages the answer and its pincites are built from. Caching only the
+// section IDs dropped them, and a repeated question came back with no
+// pincites (HAL-2846). The wrapping is intentionally transparent —
+// callers interact with the standard Strategy interface and don't know
+// caching is present.
 //
 // Cached also implements CostStrategy if the wrapped strategy does. On a
 // cache hit, CostUSD and LLMCalls are both zero (no LLM call was made).
@@ -59,19 +63,30 @@ func (c *Cached) Name() string { return c.inner.Name() }
 // Select checks the cache first. On a miss it delegates to the inner
 // strategy and caches the result.
 func (c *Cached) Select(ctx context.Context, t *tree.Tree, query string, budget ContextBudget) ([]tree.SectionID, error) {
+	// A strategy that reports a full Result is always cached through
+	// SelectWithCost, so no entry is ever an IDs-only stand-in that a
+	// later SelectWithCost hit would serve without its evidence pages.
+	if _, ok := c.inner.(CostStrategy); ok {
+		r, err := c.SelectWithCost(ctx, t, query, budget)
+		if err != nil {
+			return nil, err
+		}
+		return r.SelectedIDs, nil
+	}
+
 	key := cache.Key(string(t.DocumentID), query, c.inner.Name(), budget.ModelName)
 
-	if v, ok := c.cache.Get(key); ok {
-		return v.([]tree.SectionID), nil
+	if r, ok := c.get(key); ok {
+		return r.SelectedIDs, nil
 	}
 
 	ids, err := c.inner.Select(ctx, t, query, budget)
 	if err != nil {
 		return nil, err
 	}
 
-	c.cache.Set(key, ids, c.ttl)
-	return ids, nil
+	c.cache.Set(key, cloneResult(&Result{SelectedIDs: ids}), c.ttl)
+	return append([]tree.SectionID(nil), ids...), nil
 }
 
 // SelectWithCost checks the cache first. On a hit it returns zero usage
@@ -86,15 +101,17 @@ func (c *Cached) Select(ctx context.Context, t *tree.Tree, query string, budget
 func (c *Cached) SelectWithCost(ctx context.Context, t *tree.Tree, query string, budget ContextBudget) (*Result, error) {
 	key := cache.Key(string(t.DocumentID), query, c.inner.Name(), budget.ModelName)
 
-	if v, ok := c.cache.Get(key); ok {
-		ids := v.([]tree.SectionID)
-		return &Result{
-			SelectedIDs: ids,
-			Reasoning:   "cached",
-			ModelUsed:   budget.ModelName,
-			Usage:       Usage{}, // zero — no LLM call
-			TraceToken:  ComputeTraceToken(t.DocumentID, traceDocVersionV1, budget.ModelName, ids),
-		}, nil
+	if r, ok := c.get(key); ok {
+		r.Reasoning = "cached"
+		r.Usage = Usage{} // zero — no LLM call
+		r.HopsTaken = 0   // and no navigation on this request
+		if r.ModelUsed == "" {
+			r.ModelUsed = budget.ModelName
+		}
+		if r.TraceToken == "" {
+			r.TraceToken = ComputeTraceToken(t.DocumentID, traceDocVersionV1, budget.ModelName, r.SelectedIDs)
+		}
+		return r, nil
 	}
 
 	var result *Result
@@ -115,10 +132,51 @@ func (c *Cached) SelectWithCost(ctx context.Context, t *tree.Tree, query string,
 		}
 	}
 
-	c.cache.Set(key, result.SelectedIDs, c.ttl)
+	c.cache.Set(key, cloneResult(result), c.ttl)
 	return result, nil
 }
 
+// get returns a private copy of the cached Result for key, so a caller
+// that edits its result (the answer path trims evidence, for one) never
+// changes what the next hit sees.
+func (c *Cached) get(key string) (*Result, bool) {
+	v, ok := c.cache.Get(key)
+	if !ok {
+		return nil, false
+	}
+	r, ok := v.(*Result)
+	if !ok || r == nil {
+		return nil, false
+	}
+	return cloneResult(r), true
+}
+
+// cloneResult deep-copies the parts of a Result a caller could mutate:
+// its slices and map.
+func cloneResult(r *Result) *Result {
+	if r == nil {
+		return nil
+	}
+	out := *r
+	out.SelectedIDs = append([]tree.SectionID(nil), r.SelectedIDs...)
+	if r.Confidences != nil {
+		out.Confidences = make(map[tree.SectionID]float64, len(r.Confidences))
+		for k, v := range r.Confidences {
+			out.Confidences[k] = v
+		}
+	}
+	if r.PagesRead != nil {
+		out.PagesRead = make([]PageReadEntry, len(r.PagesRead))
+		for i, p := range r.PagesRead {
+			p.SectionIDs = append([]tree.SectionID(nil), p.SectionIDs...)
+			out.PagesRead[i] = p
+		}
+	}
+	out.EvidencePages = append([]EvidencePage(nil), r.EvidencePages...)
+	out.CitedPages = append([][2]int(nil), r.CitedPages...)
+	return &out
+}
+
 // SelectStream delegates to the inner strategy's SelectStream if it
 // implements StreamStrategy. Streaming results are NOT cached because
 // the stream is consumed as events arrive; the non-streaming Select path

pkg/retrieval/cached_evidence_test.go

@@ -0,0 +1,124 @@
+package retrieval_test
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/hallelx2/vectorless-engine/pkg/retrieval"
+	"github.com/hallelx2/vectorless-engine/pkg/tree"
+)
+
+// pageStrategy stands in for judgewalk: a page-based strategy whose
+// result carries evidence pages, cited pages and confidences.
+type pageStrategy struct{ calls int }
+
+func (p *pageStrategy) Name() string { return "pages" }
+
+func (p *pageStrategy) Select(ctx context.Context, t *tree.Tree, q string, b retrieval.ContextBudget) ([]tree.SectionID, error) {
+	r, err := p.SelectWithCost(ctx, t, q, b)
+	if err != nil {
+		return nil, err
+	}
+	return r.SelectedIDs, nil
+}
+
+func (p *pageStrategy) SelectWithCost(_ context.Context, t *tree.Tree, _ string, _ retrieval.ContextBudget) (*retrieval.Result, error) {
+	p.calls++
+	ids := []tree.SectionID{"sec_b"}
+	return &retrieval.Result{
+		SelectedIDs:   ids,
+		Confidences:   map[tree.SectionID]float64{"sec_b": 0.9},
+		Usage:         retrieval.Usage{LLMCalls: 3, InputTokens: 900, OutputTokens: 40, TotalTokens: 940, CostUSD: 0.0001},
+		PagesRead:     []retrieval.PageReadEntry{{StartPage: 60, EndPage: 60}},
+		EvidencePages: []retrieval.EvidencePage{{Page: 60, Title: "Cash flows", Text: "Purchases of PP&E (1,577) (1,373)", Confidence: 0.9}},
+		CitedPages:    [][2]int{{60, 60}},
+		Confidence:    0.8,
+		HopsTaken:     4,
+		TraceToken:    "tok-from-strategy",
+	}, nil
+}
+
+// A cache hit must return what the strategy found, not just the section
+// IDs. It used to rebuild a bare Result from the IDs, so the second time
+// anyone asked a question the evidence pages were gone and the answer
+// came back with no pincites (HAL-2846).
+func TestCachedHitKeepsEvidencePages(t *testing.T) {
+	inner := &pageStrategy{}
+	cached := retrieval.NewCached(inner, retrieval.CachedConfig{TTL: time.Minute})
+	tr := buildTree()
+	budget := retrieval.ContextBudget{MaxTokens: 100000, ModelName: "m"}
+
+	first, err := cached.SelectWithCost(context.Background(), tr, "capex?", budget)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// A caller mutating its result must not reach into the cache.
+	first.EvidencePages[0].Text = "mutated"
+	first.Confidences["sec_b"] = 0
+
+	hit, err := cached.SelectWithCost(context.Background(), tr, "capex?", budget)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if inner.calls != 1 {
+		t.Fatalf("second call should be a cache hit: strategy ran %d times", inner.calls)
+	}
+	if len(hit.EvidencePages) != 1 || hit.EvidencePages[0].Page != 60 || hit.EvidencePages[0].Text != "Purchases of PP&E (1,577) (1,373)" {
+		t.Fatalf("evidence pages lost or aliased on a hit: %+v", hit.EvidencePages)
+	}
+	if len(hit.CitedPages) != 1 || len(hit.PagesRead) != 1 || hit.Confidence != 0.8 || hit.Confidences["sec_b"] != 0.9 {
+		t.Fatalf("page result lost on a hit: cited=%v read=%v conf=%v confs=%v", hit.CitedPages, hit.PagesRead, hit.Confidence, hit.Confidences)
+	}
+	if hit.Usage != (retrieval.Usage{}) {
+		t.Fatalf("a hit costs nothing: usage = %+v", hit.Usage)
+	}
+	if hit.TraceToken != "tok-from-strategy" || hit.Reasoning != "cached" {
+		t.Fatalf("hit trace token %q reasoning %q", hit.TraceToken, hit.Reasoning)
+	}
+
+	if hit.HopsTaken != 0 {
+		t.Fatalf("a hit navigated nothing: HopsTaken = %d", hit.HopsTaken)
+	}
+
+	// Editing a hit's result must not reach the next hit either.
+	hit.EvidencePages[0].Text = "edited"
+	hit.CitedPages[0] = [2]int{1, 1}
+	again, err := cached.SelectWithCost(context.Background(), tr, "capex?", budget)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if again.EvidencePages[0].Text != "Purchases of PP&E (1,577) (1,373)" || again.CitedPages[0] != [2]int{60, 60} {
+		t.Fatalf("a hit's edits leaked into the cache: %+v %v", again.EvidencePages, again.CitedPages)
+	}
+
+	// Select on the same key returns the same IDs.
+	ids, err := cached.Select(context.Background(), tr, "capex?", budget)
+	if err != nil || len(ids) != 1 || ids[0] != "sec_b" || inner.calls != 1 {
+		t.Fatalf("Select hit: ids=%v err=%v calls=%d", ids, err, inner.calls)
+	}
+}
+
+// Select may fill the cache before SelectWithCost asks for the same key.
+// The entry must still be the full result, or the later hit would answer
+// without evidence pages.
+func TestCachedSelectFirstStillCachesEvidence(t *testing.T) {
+	inner := &pageStrategy{}
+	cached := retrieval.NewCached(inner, retrieval.CachedConfig{TTL: time.Minute})
+	tr := buildTree()
+	budget := retrieval.ContextBudget{MaxTokens: 100000, ModelName: "m"}
+
+	if _, err := cached.Select(context.Background(), tr, "capex?", budget); err != nil {
+		t.Fatal(err)
+	}
+	hit, err := cached.SelectWithCost(context.Background(), tr, "capex?", budget)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if inner.calls != 1 {
+		t.Fatalf("strategy ran %d times, want 1", inner.calls)
+	}
+	if len(hit.EvidencePages) != 1 || len(hit.CitedPages) != 1 {
+		t.Fatalf("Select-first entry lost evidence: %+v %v", hit.EvidencePages, hit.CitedPages)
+	}
+}