diff --git a/scripts/fixtures/rag-offline-contract-tests.json b/scripts/fixtures/rag-offline-contract-tests.json index 4b0130a290..85cf3b27db 100644 --- a/scripts/fixtures/rag-offline-contract-tests.json +++ b/scripts/fixtures/rag-offline-contract-tests.json @@ -19,5 +19,6 @@ "tests/privacy.test.ts", "tests/private-rag-access.test.ts", "tests/upload-admission.test.ts", - "tests/privacy-ui.test.ts" + "tests/privacy-ui.test.ts", + "tests/rag-round-trip-budget.test.ts" ] diff --git a/scripts/rag-offline-contract.mjs b/scripts/rag-offline-contract.mjs index 97b12c0541..dafb16d9b2 100644 --- a/scripts/rag-offline-contract.mjs +++ b/scripts/rag-offline-contract.mjs @@ -20,6 +20,9 @@ export const requiredOfflineContractTests = Object.freeze([ "tests/private-rag-access.test.ts", "tests/upload-admission.test.ts", "tests/privacy-ui.test.ts", + // Ledger #098: pins Supabase round-trip counts on the answer path so an added + // round trip is a red gate rather than something a reviewer has to spot. + "tests/rag-round-trip-budget.test.ts", ]); export function validateOfflineContractTests(suites) { diff --git a/tests/helpers/supabase-round-trip-counter.ts b/tests/helpers/supabase-round-trip-counter.ts new file mode 100644 index 0000000000..198ae766f8 --- /dev/null +++ b/tests/helpers/supabase-round-trip-counter.ts @@ -0,0 +1,147 @@ +/** + * Counts Supabase round trips for one scenario, so adding a round trip to a hot + * path becomes a red gate instead of something a reviewer has to notice. + * + * Ledger `#098`. The latency audit's findings were argued from reading code — + * "this awaits before that", "these two could overlap" — and the fixes were + * verified the same way. Nothing pinned the resulting counts, so a later + * refactor could reintroduce a round trip silently. This is the guard. + * + * **What counts as one round trip: execution, not construction.** A Supabase + * builder issues its request when it is awaited, not when `.from()` creates it. + * So the trip is recorded when the returned thenable is executed (`then` is + * invoked), and `.from()` / `.rpc()` on their own record nothing. Builder + * methods (`.select`, `.eq`, `.order`, …) are fluent and never counted. + * + * This distinction is load-bearing rather than pedantic, and the first version + * of this helper got it wrong (Codex P2, 2026-07-30). Counting at `.from()` + * would charge a trip for a query that was built and abandoned, and would charge + * only one for a builder awaited twice — which really is two requests. Both + * cases are pinned by tests. + * + * **Native promises are counted eagerly, lazy builders on execution.** A promise + * memoises: awaiting it twice performs one request but invokes `then` twice, so + * counting a promise on `then` would double-count a re-awaited one. An async stub + * that returns a promise has already issued its request by the time it returns, + * so the call itself is the right moment. Only a lazy thenable — a real + * PostgrestFilterBuilder, which re-requests on each execution — is counted when + * it executes. The rule is therefore "count when the request is issued", which + * both cases satisfy. (Refinement adopted from Codex's parallel fix on PR #1450, + * which handled this better than the first version of this file did.) + * + * **What this cannot see.** Only calls made through the wrapped client. A + * round trip issued via a different client instance, a direct `fetch`, or a + * provider SDK is invisible here — so a budget assertion is evidence about + * this client's traffic, not proof of total request cost. Say that rather than + * implying the latter. + * + * Provider-free and DB-free: this wraps whatever stub the suite already builds. + */ + +/** One recorded call. `name` is the RPC name or the table name. */ +export type SupabaseRoundTrip = { readonly kind: "rpc" | "from"; readonly name: string }; + +export interface SupabaseRoundTripCounter { + /** Every recorded trip, in call order. Order matters for admission-before-scope style checks. */ + readonly trips: readonly SupabaseRoundTrip[]; + /** Total round trips through this client. */ + total(): number; + /** Trips of one kind. */ + count(kind: "rpc" | "from"): number; + /** How many times a specific RPC or table was hit. */ + countOf(name: string): number; + /** `{ "rpc:match_document_chunks_text_v2": 1, "from:documents": 2 }` — for a readable failure message. */ + breakdown(): Record; + /** Drop everything recorded so far, e.g. between phases of one scenario. */ + reset(): void; +} + +// Method syntax, and `never[]` params, both deliberate: method signatures are +// bivariant and `never` is assignable to anything, so a concrete stub such as +// `(name: string) => Promise<...>` satisfies this constraint. Property syntax +// with `unknown[]` looks tidier but rejects every real stub, because a parameter +// typed `unknown` is not assignable to one typed `string`. +type MinimalSupabaseClient = { + rpc?(...args: never[]): unknown; + from?(...args: never[]): unknown; +}; + +/** + * Wraps `client` so every `.rpc()` and `.from()` is recorded, returning the + * wrapper and its counter. The wrapper delegates to the original, so the + * suite's existing stub behaviour is unchanged — this only observes. + */ +export function countSupabaseRoundTrips( + client: T, +): { client: T; counter: SupabaseRoundTripCounter } { + const trips: SupabaseRoundTrip[] = []; + const record = (kind: "rpc" | "from", name: unknown) => { + trips.push({ kind, name: typeof name === "string" ? name : String(name) }); + }; + + // Spread rather than mutate: the caller's stub may be reused by another + // scenario in the same file, and silently attaching counters to it would + // make the two scenarios share state. + const wrapped = { ...client } as T; + + /** + * Wraps a builder (or promise) so the trip is recorded when it executes. + * Fluent methods return the builder, so their results are re-wrapped to keep + * the pending trip attached however long the chain gets. + */ + const trackExecution = (target: unknown, kind: "rpc" | "from", name: unknown): unknown => { + if (target === null || (typeof target !== "object" && typeof target !== "function")) return target; + // A native promise has already issued its request; count it now and hand it + // back untouched, so re-awaiting it cannot inflate the count. + if (target instanceof Promise) { + record(kind, name); + return target; + } + return new Proxy(target as object, { + get(obj, prop, receiver) { + const value = Reflect.get(obj, prop, receiver); + if (typeof value !== "function") return value; + + if (prop === "then") { + // Execution. Record once per execution, so two awaits are two trips. + return (...args: unknown[]) => { + record(kind, name); + return (value as (...a: unknown[]) => unknown).apply(obj, args); + }; + } + return (...args: unknown[]) => { + const result = (value as (...a: unknown[]) => unknown).apply(obj, args); + // Fluent link (`return this`) or a derived builder — keep tracking. + return result === obj || (result !== null && typeof result === "object" && "then" in (result as object)) + ? trackExecution(result, kind, name) + : result; + }; + }, + }); + }; + + for (const method of ["rpc", "from"] as const) { + if (typeof client[method] !== "function") continue; + const original = (client[method] as (...args: unknown[]) => unknown).bind(client); + (wrapped as MinimalSupabaseClient)[method] = ((...args: unknown[]) => + trackExecution(original(...args), method, args[0])) as T[typeof method]; + } + + const counter: SupabaseRoundTripCounter = { + trips, + total: () => trips.length, + count: (kind) => trips.filter((trip) => trip.kind === kind).length, + countOf: (name) => trips.filter((trip) => trip.name === name).length, + breakdown: () => + trips.reduce>((acc, trip) => { + const key = `${trip.kind}:${trip.name}`; + acc[key] = (acc[key] ?? 0) + 1; + return acc; + }, {}), + reset: () => { + trips.length = 0; + }, + }; + + return { client: wrapped, counter }; +} diff --git a/tests/rag-round-trip-budget.test.ts b/tests/rag-round-trip-budget.test.ts new file mode 100644 index 0000000000..47a6ff8315 --- /dev/null +++ b/tests/rag-round-trip-budget.test.ts @@ -0,0 +1,279 @@ +/** + * Pins Supabase round-trip counts for the offline answer path (ledger `#098`). + * + * The 2026-07-28 latency audit argued every finding from reading code, and the + * fixes that landed in PR #1377 were verified the same way. Nothing pinned the + * resulting counts, so a later refactor could add a round trip to a hot path and + * no gate would notice. These budgets exist to make that a red gate. + * + * **The numbers here are measured, not derived.** They record what the path + * currently does. A failure therefore means "the traffic changed" — which is + * information, not automatically a defect. The correct response is to look at + * the printed breakdown and decide whether the change is intended, then update + * the budget in the same commit as the change that moved it. Do not relax a + * budget to make an unexplained failure pass; that converts this guard back into + * the inference it replaced. + * + * Scope: what travels through the mocked `@/lib/supabase/admin` client. Traffic + * via another client, a direct `fetch`, or a provider SDK is invisible here — so + * these are claims about this client's round trips, not total request cost. + * + * Provider-free and DB-free. + */ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { countSupabaseRoundTrips } from "./helpers/supabase-round-trip-counter"; +import type { SearchResult } from "../src/lib/types"; + +function source(overrides: Partial = {}): SearchResult { + return { + id: "clozapine-chunk-1", + document_id: "clozapine-doc", + title: "Clozapine Prescribing Administration Monitoring", + file_name: "CG.MHSP.ClozapinePresAdminMonitor.pdf", + page_number: 11, + chunk_index: 0, + section_heading: "Monitoring", + content: + "Withhold clozapine if the absolute neutrophil count (ANC) falls below 1.5 x10^9/L. Mandatory FBC monitoring is weekly for the first 18 weeks of clozapine treatment, then reduces in frequency.", + image_ids: [], + similarity: 0.95, + hybrid_score: 0.95, + text_rank: 1.2, + table_facts: [], + source_metadata: { + source_title: "Clozapine source", + publisher: "Local service", + jurisdiction: "Australia/WA", + version: "1", + publication_date: null, + review_date: null, + uploaded_at: null, + indexed_at: null, + uploaded_by: null, + document_status: "current", + clinical_validation_status: "approved", + extraction_quality: "good", + }, + images: [], + ...overrides, + }; +} + +/** Mirrors tests/rag-offline-answer.test.ts's harness, with the client counted. */ +async function answerWithCountedClient(query: string, textSources: SearchResult[]) { + vi.stubEnv("RAG_PROVIDER_MODE", "offline"); + vi.stubEnv("RAG_SEARCH_CACHE_TTL_MS", "0"); + vi.stubEnv("RAG_ANSWER_CACHE_TTL_MS", "0"); + + class EmptyQuery implements PromiseLike<{ data: unknown[]; error: null }> { + select() { + return this; + } + in() { + return this; + } + eq() { + return this; + } + neq() { + return this; + } + order() { + return this; + } + limit() { + return this; + } + is() { + return this; + } + or() { + return this; + } + abortSignal() { + return this; + } + maybeSingle() { + return this; + } + then( + onfulfilled?: ((value: { data: unknown[]; error: null }) => TResult1 | PromiseLike) | null, + ): PromiseLike { + return Promise.resolve({ data: [], error: null }).then(onfulfilled); + } + } + + const rpc = vi.fn(async (name: string) => { + if (name === "match_document_chunks_text_v2" || name === "match_document_chunks_text") { + return { data: textSources, error: null }; + } + return { data: [], error: null }; + }); + + const { client, counter } = countSupabaseRoundTrips({ rpc, from: vi.fn(() => new EmptyQuery()) }); + vi.doMock("@/lib/supabase/admin", () => ({ createAdminClient: () => client })); + vi.doMock("@/lib/openai", () => ({ + embedTextWithTelemetry: vi.fn(), + generateStructuredTextResult: vi.fn(), + })); + + const { answerQuestionWithScope } = await import("../src/lib/rag/rag"); + const answer = await answerQuestionWithScope({ + query, + ownerId: undefined, + logQuery: false, + skipCache: true, + }); + return { answer, counter }; +} + +afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); + vi.unstubAllEnvs(); +}); + +describe("the counter itself", () => { + // A budget guard is only worth having if it counts what actually goes over the + // wire. Proving that on the real path would mean editing src/lib/rag/** to + // inject calls, so these pin the mechanism instead — and they are the exact + // two cases the first version of this helper got wrong: a builder that is + // never executed must cost nothing, and one executed twice must cost two. + /** Minimal stand-in for a Supabase builder: fluent, and executes on `then`. */ + const builder = (table: string) => { + const self = { + select: () => self, + eq: () => self, + // Echoes the table back so the parameter is genuinely used, and so a + // mis-wired proxy that swallowed arguments would show up here. + then: (onfulfilled?: (value: { data: unknown[]; error: null; table: string }) => T) => + Promise.resolve({ data: [], error: null, table }).then(onfulfilled), + }; + return self; + }; + + it("charges nothing for a builder that is never executed", async () => { + const { client, counter } = countSupabaseRoundTrips({ from: vi.fn(builder) }); + + // Built and abandoned: no request was ever sent, so no trip may be charged. + // Counting at `.from()` — the first version of this helper — got this wrong. + client.from!("documents").select().eq(); + expect(counter.total(), "an unexecuted builder must cost zero round trips").toBe(0); + }); + + it("charges two round trips for a builder executed twice", async () => { + const { client, counter } = countSupabaseRoundTrips({ from: vi.fn(builder) }); + const query = client.from!("documents").select(); + + await query; + expect(counter.total(), "first execution is one trip").toBe(1); + await query; + expect(counter.total(), "re-executing a builder really does re-request").toBe(2); + expect(counter.breakdown()).toEqual({ "from:documents": 2 }); + }); + + it("counts an executed rpc and does not mutate the client it wraps", async () => { + const original = { + rpc: vi.fn(async (name: string) => ({ data: [], error: null, name })), + from: vi.fn(builder), + }; + const { client, counter } = countSupabaseRoundTrips(original); + + await client.rpc!("match_document_chunks_text_v2"); + expect(counter.total()).toBe(1); + expect(counter.countOf("match_document_chunks_text_v2")).toBe(1); + + // Spread-not-mutate: a suite reusing the same stub for a second scenario + // must not inherit the first scenario's counting. + expect(original.from, "wrapping must not replace the original's methods").not.toBe(client.from); + }); +}); + +describe("Supabase round-trip budgets on the offline answer path", () => { + it("pins the round-trip count for a single-source source-only answer", async () => { + const { answer, counter } = await answerWithCountedClient("What ANC threshold should withhold clozapine?", [ + source(), + ]); + + // Non-vacuity first. A budget of "0 observed, 0 expected" would pass while + // proving the path never ran — the failure mode that makes a guard useless. + // Assert the scenario actually produced a grounded answer and actually + // talked to the client before trusting any count. + expect(answer.grounded, "scenario must produce a grounded answer, or the budget measures nothing").toBe(true); + expect(counter.total(), "scenario must issue at least one round trip").toBeGreaterThan(0); + expect(counter.countOf("match_document_chunks_text_v2"), "text retrieval RPC must have run").toBeGreaterThan(0); + + // Measured budget. Update deliberately, in the same commit as any change + // that moves it, and say why in that commit. + expect(counter.total(), `round-trip budget exceeded — breakdown: ${JSON.stringify(counter.breakdown())}`).toBe( + OFFLINE_SINGLE_SOURCE_ROUND_TRIPS, + ); + }); + + it("does not scale round trips with the number of retrieved sources", async () => { + // The property worth guarding is shape, not magnitude: hydration and + // metadata lookups must stay batched. A per-source round trip is the + // regression this catches, and it is invisible to a single-source budget. + // + // Each source gets a DISTINCT document_id and page (Codex P2, 2026-07-30). + // The first version varied only the chunk id, so all five shared one + // document — and a regression hydrating once per distinct document would + // still have been a single call, letting this "no scaling" claim pass over + // an N+1 path on realistic cross-document results. + const many = Array.from({ length: 5 }, (_, index) => + source({ + id: `clozapine-chunk-${index + 1}`, + document_id: `clozapine-doc-${index + 1}`, + page_number: 11 + index, + chunk_index: index, + }), + ); + // Guard the fixture itself, so it cannot quietly revert to five chunks of one + // document and take this assertion back to proving nothing (suggested by + // Codex on this PR — the same non-vacuity discipline as the budgets below). + expect( + new Set(many.map((entry) => entry.document_id)).size, + "fixture must span five distinct documents, or cross-document batching is untested", + ).toBe(5); + + const { answer, counter } = await answerWithCountedClient("What ANC threshold should withhold clozapine?", many); + + expect(answer.grounded, "scenario must produce a grounded answer").toBe(true); + expect(counter.total(), "scenario must issue at least one round trip").toBeGreaterThan(0); + expect( + counter.total(), + `five sources must not cost more round trips than one — breakdown: ${JSON.stringify(counter.breakdown())}`, + ).toBe(OFFLINE_SINGLE_SOURCE_ROUND_TRIPS); + }); +}); + +/** + * Measured on 2026-07-30 against the offline source-only path. Not a target and + * not derived from first principles — it is what the path does today. See the + * file header before changing it. + * + * The 13 break down as: + * + * from:rag_aliases 1 + * rpc:match_document_chunks_text_v2 3 + * rpc:get_related_document_metadata_v2 2 + * from:document_index_quality 1 + * from:document_images 3 + * rpc:match_document_table_facts_text_v2 3 + * + * It was 14 until the counter moved from construction to execution (Codex P2, + * 2026-07-30). `document_memory_cards` is *built but never executed* on this + * path, so it sends nothing and must not be charged. That is a small finding in + * its own right — a constructed-and-abandoned builder — but harmless at runtime; + * what mattered is that the original budget over-counted real traffic by one. + * + * Two things in that breakdown are worth a reader's attention rather than being + * silently encoded as "correct". Three calls to each of the text-chunk, + * table-fact and image lookups for a single query is a repeated-triple shape, + * and `#101` records exactly that pattern — metadata/memory/visual hydration + * repeated across branches while `rag.ts` already parallelises three RPCs + * elsewhere. This budget does not endorse the count; it stops it growing + * unnoticed and gives `#101` a number to improve against. + */ +const OFFLINE_SINGLE_SOURCE_ROUND_TRIPS = 13;