diff --git a/pkg/retrieval/cached.go b/pkg/retrieval/cached.go index 1e951b1..8f337fe 100644 --- a/pkg/retrieval/cached.go +++ b/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,10 +63,21 @@ 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) @@ -70,8 +85,8 @@ func (c *Cached) Select(ctx context.Context, t *tree.Tree, query string, budget 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 diff --git a/pkg/retrieval/cached_evidence_test.go b/pkg/retrieval/cached_evidence_test.go new file mode 100644 index 0000000..38cac7e --- /dev/null +++ b/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) + } +}