From 912407bef1343cf8cb04c8beb9dbec0fb34ac780 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:49:47 +0900 Subject: [PATCH] fix(search): per-call token-budget meta masked the real cut on both cache paths; restore vacuous search-lite coverage (#2954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(search): per-call token-budget meta no longer masks the real cut; restore vacuous search-lite coverage The search-lite integration tests were structurally vacuous: putPage never creates chunks and searchKeyword joins content_chunks, so every fixture query returned zero rows on every machine. The tight-budget cut test's defensive skip ('keyword search may dedupe by page') silently returned before its assertions had ever executed anywhere, and the two budget-meta tests ran against empty result sets. Restoring the fixture (upsertChunks per page) and hardening the assertions immediately exposed a real meta bug: with a per-call tokenBudget, the inner hybridSearch enforces the same resolved budget (per-call wins in resolveSearchMode) and its meta carries the true dropped count — but hybridSearchCached re-applies the budget to the already-cut set and published THAT pass's meta, which always reads dropped=0. onMeta consumers saw a budget record claiming nothing was dropped while rows were; telemetry (recorded from the inner meta) disagreed with the caller-visible meta. - hybrid.ts finalMeta: prefer innerMeta.token_budget when a per-call budget is set (outer budgetMeta stays as the fallback and remains the enforcement for the cache-HIT path, where no inner run exists) - test fixture: chunk each page (pattern: chunk-grain-fts.test.ts) - cut test: defensive skip replaced with a hard >=2 precondition; results non-empty + strictly-fewer-than-unbounded + dropped>0 now actually execute (revert-checked red on the unfixed meta) - budget-meta tests: assert non-empty result sets so kept=results.length can no longer pass vacuously at 0=0 The unbounded 'builder' query returns 2 of 3 fixture pages by design — dedup Layer 3 caps any single page type at 60% of results and the fixture is all-person — which the >=2 precondition accommodates. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT * fix(search): cache-HIT budget meta prefers the stored cut record per review; exact-count assertions, mixed-type fixture - hit path had the symmetric masking (codex P2): the re-application runs on the already-trimmed stored set and read dropped=0 while the miss that produced the same result set reported the real cut. Prefer hit.meta.token_budget unconditionally — tokenBudget is folded into knobsHash ('tb='), so a hit only ever serves a lookup with the identical resolved budget as the write and the outer pass can never cut further (verified against mode.ts; this is why the reviewer's 'outer wins when it drops' branch is unreachable). budgetMeta remains the fallback for legacy rows stored without a budget record - new serial test drives a real store-then-hit roundtrip (mocked embedQuery, real PGLite cache) and pins hit token_budget == miss token_budget; revert-checked red (dropped=0) on the unfixed path - lite test: dropped asserted as the exact unbounded-minus-kept count and used>0 (dropped>0 alone accepts any wrong positive; used<=250 alone accepts a bogus zero), fixture types mixed (person/company/note) so dedup Layer-3 type-diversity policy no longer shapes the test Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT --------- Co-authored-by: Claude Fable 5 --- src/core/search/hybrid.ts | 20 ++- ...brid-cached-hit-budget-meta.serial.test.ts | 133 ++++++++++++++++++ test/hybrid-search-lite.serial.test.ts | 47 +++++-- 3 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 test/hybrid-cached-hit-budget-meta.serial.test.ts diff --git a/src/core/search/hybrid.ts b/src/core/search/hybrid.ts index 4f2feb856..e0a6fcb40 100644 --- a/src/core/search/hybrid.ts +++ b/src/core/search/hybrid.ts @@ -1767,8 +1767,17 @@ export async function hybridSearchCached( ...(hit.meta?.embedding_column ? { embedding_column: hit.meta.embedding_column } : {}), ...(hit.meta?.adaptive_return ? { adaptive_return: hit.meta.adaptive_return } : {}), ...(hit.meta?.autocut ? { autocut: hit.meta.autocut } : {}), + // Per-call budget: prefer the STORED budget record, which carries + // the true dropped count from the write-time cut — the + // re-application above ran on an already-cut set and reads + // dropped=0 (same masking as the miss path's finalMeta). Safe + // unconditionally: tokenBudget is folded into knobsHash (`tb=`), + // so a hit only ever serves a lookup with the identical resolved + // budget as the write — the outer pass can never cut further. + // budgetMeta stays as the fallback for legacy rows stored without + // a budget record. ...(opts?.tokenBudget && opts.tokenBudget > 0 - ? { token_budget: budgetMeta } + ? { token_budget: hit.meta?.token_budget ?? budgetMeta } : {}), }; try { @@ -1836,8 +1845,15 @@ export async function hybridSearchCached( ...(innerMeta?.embedding_column ? { embedding_column: innerMeta.embedding_column } : {}), ...(innerMeta?.adaptive_return ? { adaptive_return: innerMeta.adaptive_return } : {}), ...(innerMeta?.autocut ? { autocut: innerMeta.autocut } : {}), + // Per-call budget: prefer the INNER meta's budget record. The inner + // hybridSearch already enforced the same resolved budget (per-call wins + // in resolveSearchMode), so the re-application above sees an + // already-cut set and its meta reads dropped=0 — masking the real cut + // from onMeta consumers (the `dropped` under-report the restored + // search-lite test caught). The outer pass stays as the enforcement + // for the cache-HIT path, where no inner run exists. ...(opts?.tokenBudget && opts.tokenBudget > 0 - ? { token_budget: budgetMeta } + ? { token_budget: innerMeta?.token_budget ?? budgetMeta } : {}), }; try { diff --git a/test/hybrid-cached-hit-budget-meta.serial.test.ts b/test/hybrid-cached-hit-budget-meta.serial.test.ts new file mode 100644 index 000000000..b1b86424a --- /dev/null +++ b/test/hybrid-cached-hit-budget-meta.serial.test.ts @@ -0,0 +1,133 @@ +/** + * Cache-HIT budget-meta provenance — companion to the miss-path fix. + * + * With a per-call tokenBudget, the miss path stores an already-budgeted + * result set; a subsequent HIT re-applies the same budget to that trimmed + * payload (a structural no-op: tokenBudget is folded into knobsHash, so a + * hit only ever serves a lookup with the identical resolved budget as the + * write) — and pre-fix published that no-op pass's meta, reporting + * dropped=0 while the miss that produced the very same result set reported + * the real cut. This file drives a real store→hit roundtrip (mocked + * `embedQuery` for a deterministic vector, real PGLite SemanticQueryCache) + * and pins that the hit's token_budget matches the miss's. + * + * Serial: mock.module + gateway/global-env mutation (isolation guard R2). + */ + +import { afterAll, beforeAll, describe, expect, mock, test } from 'bun:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import * as realEmbedding from '../src/core/embedding.ts'; + +/** Deterministic 1536d unit vector — identical for every call, so the + * second consult matches the first write at cosine 1.0. */ +function fixedEmbedding(): Float32Array { + const arr = new Float32Array(1536); + for (let i = 0; i < 1536; i++) arr[i] = Math.sin(1 + i * 0.001); + let norm = 0; + for (let i = 0; i < 1536; i++) norm += arr[i] * arr[i]; + norm = Math.sqrt(norm); + if (norm > 0) for (let i = 0; i < 1536; i++) arr[i] /= norm; + return arr; +} + +// Mock BEFORE importing hybrid.ts (spread keeps every other export live). +mock.module('../src/core/embedding.ts', () => ({ + ...realEmbedding, + embed: async () => fixedEmbedding(), + embedQuery: async () => fixedEmbedding(), +})); + +// Import AFTER mocking. +const { hybridSearchCached, awaitPendingSearchCacheWrites } = + await import('../src/core/search/hybrid.ts'); +const { configureGateway, resetGateway } = await import('../src/core/ai/gateway.ts'); +const { PGLiteEngine } = await import('../src/core/pglite-engine.ts'); + +let engine: InstanceType; +let tmpHome: string; +const savedGbrainHome = process.env.GBRAIN_HOME; + +beforeAll(async () => { + // Hermetic config home so the developer's real ~/.gbrain/config.json + // can't leak an embedding_model that flips the cache consult to + // 'disabled' via isCacheSafe. + tmpHome = mkdtempSync(join(tmpdir(), 'gbrain-hit-budget-meta-')); + process.env.GBRAIN_HOME = tmpHome; + + // Pin the gateway to a 1536d provider BEFORE initSchema so the + // query_cache.embedding column is sized for the mock vectors. The fake + // key is never used — embedQuery is mocked above. + resetGateway(); + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: 1536, + env: { OPENAI_API_KEY: 'sk-fake' }, + }); + + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + + // Three keyword-findable pages, ~200 tokens each, mixed types so dedup's + // type-diversity layer keeps all of them. putPage never chunks — + // searchKeyword joins content_chunks, so chunks are explicit. + const longText = 'x'.repeat(800); + const fixtures: Array<[string, string, string]> = [ + ['alice-foo', 'Alice Foo', 'person'], + ['bob-bar', 'Bob Bar', 'company'], + ['carol-baz', 'Carol Baz', 'note'], + ]; + for (const [slug, title, type] of fixtures) { + const truth = `${title} is a builder. ${longText}`; + await engine.putPage(slug, { type, title, compiled_truth: truth }); + await engine.upsertChunks(slug, [ + { chunk_index: 0, chunk_text: truth, chunk_source: 'compiled_truth' }, + ]); + } +}); + +afterAll(async () => { + if (savedGbrainHome === undefined) delete process.env.GBRAIN_HOME; + else process.env.GBRAIN_HOME = savedGbrainHome; + try { await engine.disconnect(); } catch { /* ignore */ } + resetGateway(); + try { rmSync(tmpHome, { recursive: true, force: true }); } catch { /* ignore */ } +}); + +describe('cache HIT — token_budget provenance', () => { + test('hit reports the same cut the miss reported, not the no-op re-application', async () => { + // Miss: budget 250 keeps ~1 of 3 rows (~209 tokens each); the meta + // carries the real cut from the inner enforcement. + let missMeta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const missResults = await hybridSearchCached(engine, 'builder', { + limit: 10, + tokenBudget: 250, + onMeta: (m) => { missMeta = m; }, + }); + expect(missResults.length).toBeGreaterThan(0); + expect(missMeta?.cache?.status).toBe('miss'); + expect(missMeta?.token_budget?.budget).toBe(250); + const missDropped = missMeta?.token_budget?.dropped; + expect(missDropped).toBeGreaterThan(0); + + await awaitPendingSearchCacheWrites(); + + // Hit: identical query + knobs (tokenBudget is part of knobsHash, so + // this is the ONLY kind of lookup the stored row can serve). The + // published budget record must match the miss's — pre-fix it was the + // outer no-op pass's meta with dropped=0. + let hitMeta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const hitResults = await hybridSearchCached(engine, 'builder', { + limit: 10, + tokenBudget: 250, + onMeta: (m) => { hitMeta = m; }, + }); + expect(hitMeta?.cache?.status).toBe('hit'); + expect(hitResults.length).toBe(missResults.length); + expect(hitMeta?.token_budget?.budget).toBe(250); + expect(hitMeta?.token_budget?.dropped).toBe(missDropped); + expect(hitMeta?.token_budget?.kept).toBe(missMeta?.token_budget?.kept); + }); +}); diff --git a/test/hybrid-search-lite.serial.test.ts b/test/hybrid-search-lite.serial.test.ts index 857db692c..f3705a7c1 100644 --- a/test/hybrid-search-lite.serial.test.ts +++ b/test/hybrid-search-lite.serial.test.ts @@ -41,7 +41,11 @@ beforeAll(async () => { { slug: 'bob-bar', page: { - type: 'person', + // Mixed types across the fixture keep dedup Layer 3 (no page type + // above 60% of results) out of this test's way — an all-person set + // would be capped to 2 of 3 and couple these assertions to the + // diversity policy. + type: 'company', title: 'Bob Bar', compiled_truth: `Bob Bar is a builder. ${longText}`, }, @@ -49,7 +53,7 @@ beforeAll(async () => { { slug: 'carol-baz', page: { - type: 'person', + type: 'note', title: 'Carol Baz', compiled_truth: `Carol Baz is a builder. ${longText}`, }, @@ -57,6 +61,13 @@ beforeAll(async () => { ]; for (const p of pages) { await engine.putPage(p.slug, p.page); + // putPage never chunks — searchKeyword joins content_chunks, so a + // page without explicit chunks is invisible to the keyword arm and + // every result-dependent assertion below runs against an empty set. + // (Pattern: test/chunk-grain-fts.test.ts.) + await engine.upsertChunks(p.slug, [ + { chunk_index: 0, chunk_text: p.page.compiled_truth!, chunk_source: 'compiled_truth' }, + ]); } // Force keyword-only fallback by unsetting the embedding provider key. delete process.env.OPENAI_API_KEY; @@ -103,10 +114,10 @@ describe('hybridSearchCached \u2014 token budget', () => { limit: 10, onMeta: (m) => { meta = m; }, }); - // Don't assert non-empty here — keyword tokenization depends on the - // pglite analyzer config. What matters: meta is shaped right and - // budget metadata is absent when budget isn't set. - expect(results).toBeDefined(); + // Non-empty matters: pre-fix the fixture had no chunks, so this ran + // against an empty result set and the absent-budget assertion was + // trivially true. + expect(results.length).toBeGreaterThan(0); expect(meta?.token_budget).toBeUndefined(); }); @@ -117,19 +128,21 @@ describe('hybridSearchCached \u2014 token budget', () => { tokenBudget: 250, onMeta: (m) => { meta = m; }, }); + expect(results.length).toBeGreaterThan(0); expect(meta?.token_budget).toBeDefined(); expect(meta?.token_budget?.budget).toBe(250); expect(meta?.token_budget?.kept).toBe(results.length); }); test('tight budget cuts the result set', async () => { - // First find out the result count without a budget so the assertion - // is robust to the fixture’s actual chunking. + // All three fixture pages match 'builder' (mixed types, so dedup's + // type-diversity layer keeps all of them), and the unbounded set MUST + // have enough rows for the cut to be observable. Pre-fix this was a + // silent `return` when fewer than 2 rows came back — and with no + // chunks in the fixture, zero rows ALWAYS came back, so the cut + // assertions below had never executed anywhere. const unbounded = await hybridSearchCached(engine, 'builder', { limit: 10 }); - // Skip the cut test if the fixture happens to return only one row - // (keyword search may dedupe by page); the budget enforcement itself - // is exhaustively unit-tested in test/token-budget.test.ts. - if (unbounded.length < 2) return; + expect(unbounded.length).toBeGreaterThanOrEqual(2); let meta: HybridSearchMeta | undefined; const results = await hybridSearchCached(engine, 'builder', { @@ -137,10 +150,16 @@ describe('hybridSearchCached \u2014 token budget', () => { tokenBudget: 250, // enough for ~1 row of fixture data onMeta: (m) => { meta = m; }, }); + expect(results.length).toBeGreaterThan(0); + expect(results.length).toBeLessThan(unbounded.length); expect(meta?.token_budget?.budget).toBe(250); expect(meta?.token_budget?.kept).toBe(results.length); - expect(meta?.token_budget?.dropped).toBeGreaterThan(0); - // The budget must hold: cumulative cost <= budget. + // Exact accounting: every row the budget removed is a reported drop — + // dropped > 0 alone would accept any wrong positive count (codex). + expect(meta?.token_budget?.dropped).toBe(unbounded.length - results.length); + // The budget must hold with a real (non-zero) cost: cumulative cost + // <= budget, and used=0 would mean the accounting never ran. + expect(meta?.token_budget?.used).toBeGreaterThan(0); expect(meta?.token_budget?.used).toBeLessThanOrEqual(250); }); });