diff --git a/docs/branch-review-records/3d45d96c0c8e97d835c1a3f6c12b320255c0164dc545dce172e67703b2be6fda.record.md b/docs/branch-review-records/3d45d96c0c8e97d835c1a3f6c12b320255c0164dc545dce172e67703b2be6fda.record.md new file mode 100644 index 0000000000..9a3057fb96 --- /dev/null +++ b/docs/branch-review-records/3d45d96c0c8e97d835c1a3f6c12b320255c0164dc545dce172e67703b2be6fda.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/rag-zod-hardening-tranche1 | 1fb98a174e815ce2d7e105d9339d766d3f54b02b | PR #1946 review-and-fix | fixed PR-introduced maintainability blocker; strengthened retrieval provenance and visual shape validation; merged latest main | offline: git diff --check; Prettier changed files; check:maintainability-budgets; branch-review-ledger; ledger-write-discipline; docs:check-links; CI pending | diff --git a/docs/branch-review-records/bb4cf300c0917336cbfb205dc3631242f4e32aab35d3d3bfe1d0654c41f7bf34.record.md b/docs/branch-review-records/bb4cf300c0917336cbfb205dc3631242f4e32aab35d3d3bfe1d0654c41f7bf34.record.md new file mode 100644 index 0000000000..c0c42f5a5d --- /dev/null +++ b/docs/branch-review-records/bb4cf300c0917336cbfb205dc3631242f4e32aab35d3d3bfe1d0654c41f7bf34.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/rag-zod-hardening-tranche1 | 0fded0878b6f8e35ef50b8c97a63af6e78e1f716 | rag.ts retrieval RPC row Zod shape contract (ledger #212 tranche 1) | Approved — 4 unchecked as SearchResult[] casts on retrieval RPC rows replaced with assertRetrievalRows; no ranking/ordering/scoring logic touched | verify:pr-local (all steps green except pre-existing check:medication-lexicon-report staleness on main); test 602 files/6517 passed; typecheck exit 0; eval:rag:offline 36 golden cases 23 suites, 579 passed | diff --git a/src/lib/rag/rag-row-contracts.ts b/src/lib/rag/rag-row-contracts.ts new file mode 100644 index 0000000000..566ce5cf53 --- /dev/null +++ b/src/lib/rag/rag-row-contracts.ts @@ -0,0 +1,129 @@ +import { z } from "zod"; +import { logger } from "@/lib/logger"; +import { normalizeOptionalSourceMetadata } from "@/lib/source-metadata"; +import type { SearchResult } from "@/lib/types"; + +/** + * Runtime shape contract for a row returned by a retrieval RPC. + * + * Retrieval rows are untrusted external data, not a compile-time guarantee. The RPCs are + * versioned (`*_v2` with a legacy fallback) and the live database has been observed to + * drift from the migrations in this repo — `docs/outstanding-issues.md` `#316` records ten + * retrieval RPC bodies diverging, with weekly live-drift red since 2026-07-26. Before this + * contract existed, `rag.ts` asserted those rows 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, which is the worst failure mode for a clinical + * reference surface. + * + * The schema is deliberately asymmetric: + * + * - **Strict on the ranking, citation, and evidence fields.** The required chunk identity, + * provenance, and visual fields are `not null` in `supabase/schema.sql`, so requiring them + * cannot reject a row that works today. The four score fields are `.nullish()` — absent or + * null already flows through the downstream `?? 0` handling unchanged — but a *string where + * a number belongs* is rejected, which is precisely the silent-misranking case this exists + * to catch. + * - **Loose about everything else.** Column sets genuinely differ between RPC versions + * (`retrieval_synopsis` is absent from the older base hybrid function; `document_labels` + * and `document_summary` only appear on `match_document_chunks_v2`). `z.looseObject` + * preserves unknown keys rather than stripping them, so a harmless schema difference + * never becomes an outage or silent data loss. + */ +const retrievalImageSchema = z.looseObject({ + id: z.string().min(1), + page_number: z.number().int().nullable(), + storage_path: z.string(), + caption: z.string(), +}); + +const retrievalRowSchema = z.looseObject({ + id: z.string().min(1), + document_id: z.string().min(1), + title: z.string(), + file_name: z.string(), + page_number: z.number().int().nullable(), + chunk_index: z.number().int(), + section_heading: z.string().nullable(), + content: z.string(), + image_ids: z.array(z.string()), + source_metadata: z.record(z.string(), z.unknown()).nullable(), + images: z.array(retrievalImageSchema), + similarity: z.number().nullish(), + text_rank: z.number().nullish(), + hybrid_score: z.number().nullish(), + rrf_score: z.number().nullish(), +}); + +const retrievalRowsSchema = z.array(retrievalRowSchema); + +/** Cap on reported issues; a wholesale shape change would otherwise report one per row. */ +const MAX_REPORTED_ISSUES = 5; + +/** + * Thrown when a retrieval RPC returns rows that do not satisfy the ranking contract. + * + * The message carries only Zod issue paths and codes — never a row value. Retrieval rows + * contain clinical document text, so echoing one into a log or an error response would + * leak source content past the privacy boundary that `query-privacy.ts` maintains. + */ +export class RetrievalRowShapeError extends Error { + readonly rpc: string; + readonly issues: string[]; + + constructor(rpc: string, issues: string[]) { + super(`Retrieval RPC "${rpc}" returned rows that do not match the ranking contract: ${issues.join("; ")}`); + this.name = "RetrievalRowShapeError"; + this.rpc = rpc; + this.issues = issues; + } +} + +function describeIssues(error: z.ZodError): string[] { + const described = error.issues + .slice(0, MAX_REPORTED_ISSUES) + .map((issue) => `${issue.path.join(".") || ""}: ${issue.message}`); + const remaining = error.issues.length - described.length; + return remaining > 0 ? [...described, `and ${remaining} more`] : described; +} + +/** + * Validate retrieval RPC rows before they enter the ranking pipeline. + * + * This asserts rather than transforms: on success the caller keeps the original array and + * row objects, so object identity, key order, and nested `images` / `source_metadata` + * references are unchanged from what the RPC returned. That matters because ranking is a + * live-validated protected surface (`docs/rag-behaviour/`) — validation must be observable + * only when the data is already wrong. + * + * Logs before throwing so drift stays visible even where a caller degrades: the vector + * fallback in `rag.ts` catches retrieval failures and falls back to lexical results when it + * has them, which would otherwise swallow the signal entirely. + */ +export function assertRetrievalRows(rows: unknown, rpc: string): asserts rows is SearchResult[] { + const parsed = retrievalRowsSchema.safeParse(rows); + if (parsed.success) return; + const issues = describeIssues(parsed.error); + logger.error("retrieval_row_shape_mismatch", { + rpc, + issues, + rowCount: Array.isArray(rows) ? rows.length : null, + }); + throw new RetrievalRowShapeError(rpc, issues); +} + +/** Build and validate the locally retrieved rows used as document-summary context. */ +export function buildDocumentSummaryResults( + chunks: unknown[], + document: { title: string; file_name: string; metadata?: unknown }, +): SearchResult[] { + const results = chunks.map((chunk) => ({ + ...(chunk as object), + title: document.title, + file_name: document.file_name, + source_metadata: normalizeOptionalSourceMetadata(document.metadata), + similarity: 1, + images: [], + })); + assertRetrievalRows(results, "document_summary_context"); + return results; +} diff --git a/src/lib/rag/rag.ts b/src/lib/rag/rag.ts index e09f968c1f..e16eab72e1 100644 --- a/src/lib/rag/rag.ts +++ b/src/lib/rag/rag.ts @@ -1,6 +1,7 @@ import { createAdminClient } from "@/lib/supabase/admin"; import { loadDocumentSummaryContext } from "@/lib/rag/rag-document-summary-context"; import { generationFailureDetailToken } from "@/lib/rag/rag-generation-failure-diagnostics"; +import { assertRetrievalRows, buildDocumentSummaryResults } from "@/lib/rag/rag-row-contracts"; import { answerInstructions } from "@/lib/rag/rag-answer-instructions"; import { retrievalAccessScopeForArgs, retrievalRpcScopeArgs } from "@/lib/owner-scope"; import { @@ -2179,13 +2180,17 @@ export async function searchChunksWithTelemetry( const { data: hybridData, error: hybridError } = hybridResult; if (hybridError) recordHybridRpcError(telemetry, "match_document_chunks_hybrid", hybridError); - telemetry.vector_candidate_count = hybridData?.length ?? 0; - recordRetrievalLayer(telemetry, "hybrid_vector", hybridData?.length ?? 0, { + // On a hybrid RPC error `hybridData` is null, so this validates an empty array and the + // existing hybrid-error -> vector-fallback path below is reached unchanged. + const hybridRows = hybridData ?? []; + assertRetrievalRows(hybridRows, "match_document_chunks_hybrid"); + telemetry.vector_candidate_count = hybridRows.length; + recordRetrievalLayer(telemetry, "hybrid_vector", hybridRows.length, { latencyMs: hybridResult.latencyMs, - topScore: layerTopScore((hybridData ?? []) as SearchResult[]), + topScore: layerTopScore(hybridRows), }); const vectorCandidates = mergeSearchResults( - mergeSearchResults((hybridData ?? []) as SearchResult[], embeddingFieldCandidates), + mergeSearchResults(hybridRows, embeddingFieldCandidates), indexUnitCandidates, ); @@ -2257,7 +2262,9 @@ export async function searchChunksWithTelemetry( ); if (error) throw new Error(error.message); - return (data ?? []) as SearchResult[]; + const rows = data ?? []; + assertRetrievalRows(rows, "match_document_chunks"); + return rows; }), ).catch((error) => { if (!args.forceEmbedding && textFastResults.length > 0) return [] as SearchResult[][]; @@ -4304,15 +4311,7 @@ export async function summarizeDocument(documentId: string, ownerId?: string, op } satisfies RagAnswer; } - const documentMetadata = (document as { metadata?: unknown }).metadata; - const results = committedChunks.map((chunk) => ({ - ...chunk, - title: document.title, - file_name: document.file_name, - source_metadata: normalizeOptionalSourceMetadata(documentMetadata), - similarity: 1, - images: [], - })) as SearchResult[]; + const results = buildDocumentSummaryResults(committedChunks, document); const summaryInstructions = `Summarize a clinical document for practical psychiatric use in Perth, Australia. Use only the excerpts provided. Use a layered response: make the answer field a plain high-yield clinical paragraph, diff --git a/tests/rag-retrieval-row-contract.test.ts b/tests/rag-retrieval-row-contract.test.ts new file mode 100644 index 0000000000..4aa447865f --- /dev/null +++ b/tests/rag-retrieval-row-contract.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it, vi } from "vitest"; +import { RetrievalRowShapeError, assertRetrievalRows, buildDocumentSummaryResults } from "@/lib/rag/rag-row-contracts"; + +vi.mock("@/lib/logger", () => ({ + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +// A realistic `match_document_chunks_hybrid_v2` row. Column list mirrors the RPC's +// `returns table (...)` in supabase/migrations/20260713020000_owner_plus_public_retrieval.sql. +/** A row with one column dropped, standing in for an RPC whose shape has drifted. */ +function withoutColumn(column: string, overrides: Record = {}) { + const row: Record = hybridRow(overrides); + delete row[column]; + return row; +} + +function hybridRow(overrides: Record = {}) { + return { + id: "3f1a2b6c-1111-4aaa-8bbb-000000000001", + document_id: "3f1a2b6c-2222-4aaa-8bbb-000000000002", + title: "RANZCP Mood Disorders Guideline", + file_name: "ranzcp-mood.pdf", + page_number: 14, + chunk_index: 7, + section_heading: "Lithium monitoring", + content: "Check serum lithium 5 days after any dose change.", + retrieval_synopsis: null, + image_ids: [], + source_metadata: { document_status: "current" }, + similarity: 0.91, + text_rank: 0.42, + hybrid_score: 0.88, + rrf_score: 0.031, + images: [], + ...overrides, + }; +} + +describe("retrieval row shape contract", () => { + it("accepts a realistic hybrid RPC row without mutating it", () => { + const rows: unknown = [hybridRow()]; + const before = structuredClone(rows); + const firstRowReference = (rows as unknown[])[0]; + + expect(() => assertRetrievalRows(rows, "match_document_chunks_hybrid")).not.toThrow(); + + // Assert, do not transform: ranking is a live-validated protected surface, so a valid + // row must reach it byte-identical and by the same reference. + expect(rows).toEqual(before); + expect((rows as unknown[])[0]).toBe(firstRowReference); + }); + + it("builds valid document-summary rows outside the retrieval monolith", () => { + const chunk = hybridRow({ title: "stale", file_name: "stale.pdf", similarity: 0.2 }); + + const rows = buildDocumentSummaryResults([chunk], { + title: "Current title", + file_name: "current.pdf", + metadata: { document_status: "current" }, + }); + + expect(rows[0]).toMatchObject({ + id: chunk.id, + title: "Current title", + file_name: "current.pdf", + similarity: 1, + source_metadata: expect.objectContaining({ document_status: "current" }), + }); + }); + + it("preserves unknown columns so an RPC version difference is not data loss", () => { + const rows: unknown = [hybridRow({ document_labels: [{ id: "l1" }], a_future_column: 42 })]; + + assertRetrievalRows(rows, "match_document_chunks_v2"); + + expect(rows[0]).toMatchObject({ document_labels: [{ id: "l1" }], a_future_column: 42 }); + }); + + it("rejects a numeric score returned as a string", () => { + const rows: unknown = [hybridRow({ similarity: "0.91" })]; + + expect(() => assertRetrievalRows(rows, "match_document_chunks_hybrid")).toThrow(RetrievalRowShapeError); + }); + + it("rejects a row missing its chunk identity", () => { + const rows: unknown = [withoutColumn("id")]; + + expect(() => assertRetrievalRows(rows, "match_document_chunks_hybrid")).toThrow(RetrievalRowShapeError); + expect(() => assertRetrievalRows([hybridRow({ document_id: "" })], "x")).toThrow(RetrievalRowShapeError); + }); + + it("rejects malformed provenance and visual fields before they can miscite or crash rendering", () => { + expect(() => assertRetrievalRows([withoutColumn("title")], "match_document_chunks_hybrid")).toThrow( + RetrievalRowShapeError, + ); + expect(() => + assertRetrievalRows([hybridRow({ image_ids: "not-an-array" })], "match_document_chunks_hybrid"), + ).toThrow(RetrievalRowShapeError); + expect(() => assertRetrievalRows([hybridRow({ images: [{}] })], "match_document_chunks_hybrid")).toThrow( + RetrievalRowShapeError, + ); + }); + + it("accepts absent or null scores, which downstream already coalesces to 0", () => { + expect(() => assertRetrievalRows([withoutColumn("text_rank")], "match_document_chunks")).not.toThrow(); + expect(() => assertRetrievalRows([hybridRow({ rrf_score: null })], "match_document_chunks")).not.toThrow(); + expect(() => assertRetrievalRows([], "match_document_chunks")).not.toThrow(); + }); + + it("names the RPC and leaks no row content in the error", () => { + const secret = "Check serum lithium 5 days after any dose change."; + let thrown: RetrievalRowShapeError | null = null; + try { + assertRetrievalRows([hybridRow({ hybrid_score: "0.88" })], "match_document_chunks_hybrid"); + } catch (error) { + thrown = error as RetrievalRowShapeError; + } + + expect(thrown).toBeInstanceOf(RetrievalRowShapeError); + expect(thrown?.rpc).toBe("match_document_chunks_hybrid"); + expect(thrown?.message).toContain("match_document_chunks_hybrid"); + expect(thrown?.message).toContain("hybrid_score"); + // Retrieval rows carry clinical document text; it must never reach a log or a response. + expect(thrown?.message).not.toContain(secret); + expect(thrown?.message).not.toContain("ranzcp-mood.pdf"); + }); + + it("caps the reported issues so a wholesale shape change stays readable", () => { + const rows = Array.from({ length: 20 }, () => hybridRow({ similarity: "nope" })); + + let thrown: RetrievalRowShapeError | null = null; + try { + assertRetrievalRows(rows, "match_document_chunks_hybrid"); + } catch (error) { + thrown = error as RetrievalRowShapeError; + } + + expect(thrown?.issues).toHaveLength(6); + expect(thrown?.issues.at(-1)).toBe("and 15 more"); + }); + + it("rejects a payload that is not an array of rows", () => { + expect(() => assertRetrievalRows({ rows: [] }, "match_document_chunks_hybrid")).toThrow(RetrievalRowShapeError); + expect(() => assertRetrievalRows(null, "match_document_chunks_hybrid")).toThrow(RetrievalRowShapeError); + }); +});