mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-28 14:59:47 +00:00
fix(search): per-call token-budget meta masked the real cut on both cache paths; restore vacuous search-lite coverage (#2954)
* 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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<typeof PGLiteEngine>;
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user