harden(rag): validate retrieval RPC rows against a Zod shape contract - #1946
Conversation
Ledger #212 tranche 1. `rag.ts` asserted rows returned by the retrieval RPCs straight into the ranking pipeline with a bare `as SearchResult[]`, so a renamed column or a numeric returned as a string did not fail — it misranked or mis-cited silently. That is not hypothetical: ledger #316 records ten retrieval RPC bodies on the live database diverging from the migrations in this repo, with weekly live-drift red since 2026-07-26. Add `assertRetrievalRows`, a TypeScript assertion backed by a Zod schema that is strict on what ranking reads (`id`, `document_id`, `content` and the four score fields) and loose about everything else, since column sets genuinely differ between RPC versions. It asserts rather than transforms, so a valid row reaches ranking by the same reference with its key order and nested `images` / `source_metadata` untouched. Errors carry Zod issue paths only, never a row value, because retrieval rows contain clinical document text. Applied at the four unchecked casts: the hybrid layer's telemetry and merge, the vector-fallback result set, and the document-summary context. No ranking, ordering, scoring, comparator or selection logic is touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013xVTc5bPihwqTfnr8yFwWq
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:35 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
Immutable review record travelling with its owning product PR, per AGENTS.md — review records use independent record files rather than a dedicated ledger-only branch or an edit to the frozen historical table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013xVTc5bPihwqTfnr8yFwWq
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #10820 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 14, 2026
Final review summary Final PR head: Fixed:
Review and threads:
Decisive local checks:
Exact-head CI:
Blockers/residual risk: none. The PR merged externally; I did not merge it. |
Summary
Ledger
#212tranche 1 — replace the unsafe casts on the highest-consequence file,src/lib/rag/rag.ts, with runtime shape validation.rag.tsasserted the rows returned by the retrieval RPCs straight into the ranking pipeline with a bareas SearchResult[]. That cast is a compile-time fiction: the rows are untrusted external data, the RPCs are versioned (*_v2with a legacy fallback), and ledger#316records ten retrieval RPC bodies on the live database diverging from the migrations in this repo, with weekly live-drift red since 2026-07-26. Under that cast a renamed column or a numeric returned as a string does not fail — it misranks or mis-cites silently, which is the worst failure mode for a clinical reference surface.RAG impact: no retrieval behaviour change — type-validation hardening only, no ranking/ordering/scoring logic touched.
src/lib/rag/rag-row-contracts.ts—assertRetrievalRows, a TypeScript assertion function backed by a Zod schema, plus a namedRetrievalRowShapeError. The schema is deliberately asymmetric: strict on what ranking actually reads (id,document_id,content, and the four score fields), loose about everything else viaz.looseObject, because column sets genuinely differ between RPC versions —retrieval_synopsisis absent from the older base hybrid function, anddocument_labels/document_summaryappear only onmatch_document_chunks_v2. Rejecting or stripping an unlisted column would turn a harmless schema difference into an outage or silent data loss.id,document_idandcontentarenot nullinsupabase/schema.sql, so requiring them cannot reject a row that works today; the score fields are.nullish()so an absent or null score still flows through the existing downstream?? 0handling, while a string where a number belongs is caught.src/lib/rag/rag.ts— the hybrid layer's telemetry and merge, the vector-fallback result set, and the document-summary context. Each replaces a cast with a preceding assertion; no surrounding expression changes.telemetry.vector_candidate_count = hybridRows.lengthis exactly the previoushybridData?.length ?? 0.tests/rag-retrieval-row-contract.test.ts— 8 offline cases covering the accept path, unknown-column preservation, a string score, a missing chunk identity, absent/null scores, the issue cap, non-array payloads, and a case asserting the error text leaks no row content.The assertion asserts rather than transforms. On success the caller keeps the original array and the original row objects, so object identity, key order, and the nested
images/source_metadatareferences reaching the ranking pipeline are unchanged from what the RPC returned. That property is why this is safe to land on a live-validated protected surface: validation is observable only when the data is already wrong.Errors carry Zod issue paths and codes only, never a row value — retrieval rows contain clinical document text, and echoing one into a log or an error response would leak source content past the boundary
query-privacy.tsmaintains. The assertion logs before throwing, so drift stays visible even where a caller degrades (the vector fallback's existing.catchfalls back to lexical results when it has them, which would otherwise swallow the signal).Deliberately not changed, because they are outbound serialization rather than an inbound runtime shape and a Zod schema does not apply:
rag.tsquery_embedding: embedding as unknown as string(two sites) — a pgvector argument where the generated Supabase types wantstringfor anumber[].src/app/api/search/route.tsas unknown as Json(two sites) — telemetry inserts built from local values; validating inside a fire-and-forget write would lose rows.Audited and already hardened, so untouched: all three immediate API-route callers (
api/answer,api/answer/stream,api/search) already validate request bodies withparseJsonBody(request, <zodSchema>), andparseAnswerJsonalready wrapsanswerJsonSchema.parse(JSON.parse(raw))in atry/catchthat degrades tosafeFallbackAnswer.src/lib/rag/rag-candidate-sources.tscarries the same class of cast at its own RPC boundaries and is left for tranche 2 rather than widening this PR.Verification
npm run verify:pr-local— passed every step except one pre-existing failure unrelated to this diff (below):check:runtime,check:installed-lock-parity,format:changed,lint,typecheck,test,build,eval:rag:offline,check:medication-interactions.npm run test—Test Files 602 passed (602),Tests 6517 passed | 4 skipped (6521). This includestests/rag-imputation-contract.test.ts, the repo's own source-text pins on the score-imputation formulas and the release comparator key order; those stayed green, which is the direct evidence that no ranking formula or comparator order moved.npm run typecheck— exit 0. Theasserts rows is SearchResult[]narrowing is the load-bearing type change, and it is what allows all fouras SearchResult[]casts to be deleted rather than merely guarded.npm run eval:rag:offline(run insideverify:pr-local) —Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites), thenTest Files 23 passed (23),Tests 579 passed (579).npx vitest run tests/rag-retrieval-row-contract.test.ts—Test Files 1 passed (1),Tests 8 passed (8).npm run test:focusedcorrectly refuses this file, since focused selection fails closed when test infrastructure changes.Verification not run:
npm run eval:retrieval:quality,npm run eval:rag,npm run eval:quality,npm run verify:release,npm run check:supabase-project— all provider-backed and require explicit approval, which was not given.docs/rag-behaviour/requires a live eval-canary pair for a retrieval/ranking/ordering behaviour change; this PR deletes casts and adds an assertion without touching ranking, selection, scoring, or comparator logic, so no canary pair is required.UI verification not run: no UI, routing, styling, reduced-motion, or forced-colors change — the diff is two library files and one unit test.
Pre-existing failure, not from this diff:
npm run check:medication-lexicon-reportreportsdocs/medication-interaction-lexicon-review.mdis stale. That report is generated fromsrc/lib/medication-interaction-lexicon, the medication snapshot, and the medication interaction index.git diff origin/mainfor the report and for every one of those inputs is empty on this branch, so the staleness is already onmainand is unrelated to this change. Left for whoever owns that surface rather than folded into a RAG PR.Risk and rollout
RetrievalRowShapeErrorwhere it previously returned quietly-misranked results. That is the intended trade and was chosen explicitly over dropping invalid rows. Given#316reports live RPC divergence, this is the change most likely to surface something. It cannot reject data that works today: the three required fields arenot nullin the schema, the score fields accept absent and null, andz.looseObjectpreserves every column the schema does not name. The hybrid path's existing hybrid-error → vector-fallback route and the vector fallback's existing.catchdegrade-to-lexical route are both unchanged, so two of the three call sites already sit behind a conservative fallback.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)The change adds validation in front of the ranking pipeline and alters no clinical behaviour. Citation and source-verification requirements are untouched; no document-access, owner-scope, or privacy path is modified; no Supabase target, credential handling, or demo/live separation changes; source metadata and outdated-source handling are untouched. On the last item: no clinical decision-support behaviour changed, so the classification is unaffected — the answer path returns the same answers from the same sources in the same order, and the only new outcome is a loud failure on a shape that would previously have produced a quietly wrong one, which is a move toward conservative degradation, not away from it.
Notes
Flagged to the user before any edit, per
AGENTS.md"RAG ranking protection", since the diff touchessrc/lib/rag/**even though it is type-safety hardening rather than a ranking change.The throw-on-mismatch posture was confirmed with the user rather than assumed, against the two alternatives of dropping invalid rows with a log, or throwing behind an environment kill-switch.
🤖 Generated with Claude Code
https://claude.ai/code/session_013xVTc5bPihwqTfnr8yFwWq
Generated by Claude Code