harden(api): validate inbound DB and RPC payloads against Zod row contracts - #2023
Conversation
…tracts Ledger #212 tranche 3. Tranches 1 and 2 finished src/lib/rag/**; this covers src/app/api/**, where four unchecked casts asserted a structure onto inbound database and RPC data that nothing verified at runtime. Adds src/lib/validation/row-contracts.ts, the src/app/api counterpart of src/lib/rag/rag-row-contracts.ts. It deliberately duplicates that module's small validate-log-throw core rather than importing it, so hardening an API route never edits a protected RAG ranking surface. The four sites, all now Zod-backed `asserts x is T` functions that return void so callers keep the original arrays and row objects (identity and key order are unchanged for valid data): - search_document_chunks RPC rows (documents/[id]/search). scoreChunk coerces with Number(row.text_rank ?? 0), so a stringified rank silently reordered the snippets a clinician reads instead of failing. - document_chunks table-fallback rows (same route). Here index_generation_id genuinely gates which chunks are visible. - document_labels rows (documents/[id]/labels). The previous `as DocumentLabel[]` narrowed plain `text` columns to TS unions with nothing checking them, so an out-of-union label_type reached the clinical badge surfaces. - The search_schema_health payload (setup-status), declared `Returns: Json` in the generated types, so the previous cast was a structure claim with nothing behind it at all. Every required field is `not null` or carries a `check` in supabase/schema.sql, cited per contract, so no pin can reject a row the database accepts today. document_chunks.metadata and document_labels.metadata are bare `jsonb` with no jsonb_typeof check and are therefore deliberately left unpinned. Schemas use z.looseObject so a live column set ahead of this repo is preserved, not stripped. Errors carry Zod issue paths and codes only, never row values, because these rows carry clinical document text. Updates four document_labels fixtures in tests/private-access-routes.test.ts that returned partial rows. selectLabels issues a select("*"), which really returns every column, and label_type, source and confidence are all `not null` with check constraints, so the fixtures were unrealistic rather than the contract wrong. No behaviour change for valid data. Audited and deliberately left alone: the three `as unknown as Json` casts in src/app/api are outbound telemetry serialization, and every request body already validates through parseJsonBody. Documents a finding while wiring it: search_document_chunks returns neither metadata nor index_generation_id, because migration 20260717130000_filter_search_document_chunks_committed_generation moved the committed-generation filter into the RPC body, so the route's client-side generation filter is inert on the RPC path. Left exactly as it behaves today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two immutable inbox requests; neither edits the canonical ledger, so they apply on npm run issues:reconcile after this lands. ADD (P1, issue) — should a fabricated similarity of 1 on document-summary rows earn the "high" confidence label a clinician reads? buildDocumentSummaryResults stamps similarity: 1 without setting similarity_origin, and deriveConfidence computes strongestNonSynthetic by excluding only rows tagged "synthetic_text", so the untagged fabricated score is counted and gates "high" at >= 0.82. Note the mechanism is the ABSENCE of the tag here, not its presence: the three sites that do tag synthetic scores are in rag-candidate-sources.ts. A clinical-governance decision before an engineering one; changing it needs its own design, discriminating offline tests, and a live eval-canary pair per docs/rag-behaviour/. Recorded because it previously lived only in PR #1981's body. UPDATE #212 — the row was stale twice over. Its "40 unsafe casts" headline is wrong (measured on main: 80 as-unknown-as and 60 JSON.parse, most of them legitimate outbound or client-handle casts), and the five files it named as holding the remaining 11 casts in fact hold zero of either. Replaced with the measured position: src/lib/rag/** complete, src/app/api/** covered by this PR, worker/main.ts (11) remaining as its own tranche. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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: 20 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 94 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 (9)
Comment |
Immutable review record for PR #2023; the frozen historical table is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…state Resolve the HANDOVER.md status-table conflict against main after PR #2022 (S1, squash 2bd146e) and PR #2023 (#212 T3, squash 440a34f) merged. - HANDOVER §1 snapshot re-dated to 2026-08-17: S1 landed (rung 1, canary pair green), residuals R1/R2/R3 recorded, owner decisions (R1 before S2; governance Option B), #212 sibling stream cross-referenced. - HANDOVER §2 rows: S1 merged; new S1b (R1 routing), S1c (R2+R3), G1 (Option B), #212 T1-T3 / T4 rows; §3 packet bodies and §7 prompts for S1b, S1c, G1. - COORDINATION §3 wave table re-cut (Wave 0: D1 + S4 + S1b + T4; C1 canary done), per-task model/effort table, §7 current state with the canary evidence and the live board link. - clinical-hazard-analysis H5a: partial mitigation noted; document-summary residual and the Option B decision recorded. - Six immutable inbox requests: R1 (P1), R2+R3 (P2), Sentry search error (P2), source_metadata pin (P3), eval-canary post-merge-only note (P3), G1 implementation (P3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ledger (#2045) * docs(issues): reconcile 28 inbox requests into the outstanding-issues ledger Redo of the earlier reconcile: main gained 6 more pending inbox requests (3 cancellations, 1 update, 1 update, 1 done) after this branch's original base, which check:ledger-write-discipline correctly rejected as a partial transaction. Restarted from fresh origin/main and reconciled the complete current pending batch (28 requests) in one transaction, per PRs #2023/#2024 and everything that landed on main since. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2BUayF2Z9DAxvmL5XmLcC * docs(review): record redone ledger-reconcile review Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2BUayF2Z9DAxvmL5XmLcC --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Ledger
#212tranche 3. Tranches 1 (PR #1946,rag.ts) and 2 (PR #1981,rag-candidate-sources.ts) finishedsrc/lib/rag/**for this class of defect. This coverssrc/app/api/**, and deliberately does not widen back into the RAG surface.src/lib/validation/row-contracts.tsis thesrc/app/apicounterpart ofsrc/lib/rag/rag-row-contracts.ts. It duplicates that module's small validate-log-throw core rather than importing it, so hardening an API route never edits a protected RAG ranking surface. Each contract asserts rather than transforms:asserts x is Treturning void, so callers keep the original arrays and row objects, with object identity and key order unchanged for valid data. Every schema usesz.looseObject, so a live column set ahead of this repo is preserved rather than stripped, and every error carries Zod issue paths and codes only, never row values, because these rows carry clinical document text.search_document_chunksRPC rows indocuments/[id]/search.scoreChunkcoerces withNumber(row.text_rank ?? 0), so a stringified rank silently reordered the snippets a clinician reads instead of failing.document_chunkstable-fallback rows in the same route, whereindex_generation_idgenuinely gates which chunks are visible.document_labelsrows indocuments/[id]/labels. The previousas DocumentLabel[]narrowed plaintextcolumns to TypeScript unions with nothing checking them, so an out-of-unionlabel_typereached the clinical badge surfaces.search_schema_healthpayload insetup-status, declaredReturns: Jsonin the generated types, so the previous cast was a structure claim with nothing at all behind it.not nullor carries acheckinsupabase/schema.sql, cited in the code comment for each contract, so no pin can reject a row the database accepts today.document_labels.label_typeandsourcemirror theircheckconstraints exactly, and thelabel_typelist carries asatisfies [DocumentLabelType, ...]pin so widening the union without widening the constraint fails the build.document_chunks.metadataanddocument_labels.metadataare barejsonbwith nojsonb_typeofcheck and are therefore deliberately left unpinned rather than asserted to be objects.src/app/apiwere audited. They hold only threeas unknown asoccurrences, all outbound telemetry serialization cast toJson, to which no inbound schema applies, and noJSON.parseat all. Every request body already validates throughparseJsonBody(request, <zodSchema>). The genuine targets were a different metric entirely: casts asserting structure onto inbound data. The handover estimate of 15 remediable casts here did not survive measurement.tests/private-access-routes.test.tsmockeddocument_labelsselects with partial rows (id,document_id,label).selectLabelsissues aselect("*"), which really returns every column, andlabel_type,sourceandconfidenceare allnot nullwithcheckconstraints — so the fixtures were unrealistic, not the contract wrong. They now carry semantically correct values per test, and no assertion was weakened.npm run issues:reconcileafter this lands; neither edits the canonical ledger. One adds thesimilarity_originclinical-governance question, previously recorded only in PR harden(rag): validate signal rows from the candidate-source RPCs #1981's body. One corrects#212, whose description was stale in two distinct ways: its "40 unsafe casts" headline (measured onmain: 80as unknown asand 60JSON.parse, most of them legitimate outbound or client-handle casts), and its named remaining population —src/app/api/documents/route.ts,src/app/api/ingestion/{batches,jobs,quality}/route.tsandsrc/app/api/jobs/route.tswere listed as holding 11 casts, and each in fact holds zero of either.Verification
npm run verify:pr-local— 13 steps completed green:check:runtime,check:installed-lock-parity,format:changed,sitemap:check,docs:check-index,docs:check-inventory,docs:check-scripts,docs:check-links,check:branch-review-ledger,check:outstanding-issues,check:ledger-write-discipline,lint,typecheck. The gate then halted atteston the two pre-existing failures described below, so it reported- failed: test (exit 1)and left four steps unreached. Those four were run individually afterwards and are recorded below rather than left as an unstated gap.npm run build— exit 0,Compiled successfully in 52s,Client bundle secret surface check passed.Run against a removed.nextso the bundle figures are not read from a cached build.npm run check:rag:fixtures— exit 0,Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).npm run check:medication-interactions— exit 0,data/medication-interaction-index.json is up to date (523 rows).npm run check:medication-lexicon-report— exit 0,docs/medication-interaction-lexicon-review.md is up to date (28 catalogue terms).The staleness recorded against this check in earlier tranche notes is resolved on currentmain.npx vitest run tests/api-row-contract.test.ts— 27 passed, covering shape acceptance, non-mutation and reference identity, unknown-key preservation, everylabel_typethe check constraint permits, and that error messages never echo row content.npm run test—Test Files 2 failed | 613 passed | 2 skipped (617),Tests 2 failed | 6699 passed | 27 skipped (6728).Both remaining failures are pre-existing and environment-dependent, not caused by this diff. That was proved rather than assumed: a detached worktree at
d02767184(this branch's merge base) with no changes applied reproduces both identically —Test Files 2 failed (2),Tests 2 failed | 16 passed (18).tests/session-start-hook.test.ts— asserts a Windows path is contained in a file the hook writes with POSIX separators.tests/check-playwright-browser-revision.test.ts— the known Playwright/browser-revision drift recorded as#255.npx tsc --noEmit— exit 0. Theassertsnarrowing is what lets the casts be deleted rather than merely guarded.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changedUI verification not run: no component, route, style, or browser behavior changed; the diff is server-side validation on three existing API handlers plus a new library module and its unit test.
npm run eval:retrieval:quality— not applicable and not run. No retrieval, ranking, selection, chunking, or scoring behavior changed:src/lib/rag/**is untouched, and the contracts assert without transforming, so identical row objects in identical order reach every existing consumer. It is also provider-backed and needs explicit approval.npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only— not applicable and not run. Answer generation, the synthesis prompt, and answer post-processing are unchanged. Provider-backed.npm run check:production-readiness— not run. Provider-backed, and no clinical workflow, privacy, environment, Supabase, source-governance, or deployment behavior changed.Risk and rollout
not nullorcheckconstraint already guarantees, verified field by field againstsupabase/schema.sqland cited in each contract's comment, and by leaving both unconstrainedjsonbmetadata columns unpinned — the one unbacked pin shipped in tranche 1 is exactly the mistake being avoided here.z.looseObjectmeans a live column set ahead of this repo passes rather than failing. Thesetup-statuspath already wraps its read in atry/catchthat reportsneeds_setup, so a mismatch there degrades to the same status a falsyokproduces today, with a logged issue path instead of a bare "unknown".git revertthe hardening commit; it is self-contained and the ledger commit is independently revertible. The new module has no other importers, and reverting restores the previous casts exactly.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Governance notes for the items above. Citation and source verification are untouched: the contracts sit below the citation layer and assert without transforming, so the same rows reach the same renderers. No document-access, owner-scope, or privacy boundary moved; the owner-scoped queries and
withOwnerReadScopewrappers are unchanged, and error messages deliberately carry Zod issue paths and codes only, never row values, so clinical document text cannot leak into a log or a response. No Supabase target, key handling, or service-role usage changed. Demo-mode branches are untouched. Source metadata and review status are unchanged, and failure behavior degrades conservatively rather than guessing — a shape mismatch throws to the route's existing error handler instead of scoring a malformed row. On SaMD/TGA: no clinical decision-support behavior changed, so classification is unaffected. Valid data produces byte-identical output.Notes
A finding documented while wiring, not fixed here.
search_document_chunksreturns neithermetadatanorindex_generation_id— migration20260717130000_filter_search_document_chunks_committed_generation.sqlmoved the committed-generation filter into the RPC body itself (schema.sql:4047). The route's client-sideisCommittedGenerationMetadatapass over those rows therefore always fails open and is inert on the RPC path; it is load-bearing only on the table fallback, which selects both columns explicitly. That is left exactly as it behaves today and recorded in the contract's comment. It is dead code, not wrong code, and removing it is a behavior question for its own change.#212is not closeable by this PR.worker/main.tsholds 11as unknown asoccurrences and is the next and largest cluster, deliberately deferred because ingestion is a separate risk domain. Scripts are not production paths.The
similarity_originitem added to the ledger is worth more than this PR: it is the only open item in this line of work that changes what a clinician reads. It was excluded from PR harden(rag): validate signal rows from the candidate-source RPCs #1981 and from this one rather than bundled, because it is a clinical-governance decision that needs its own design, discriminating offline tests, and a live eval-canary pair perdocs/rag-behaviour/.Its mechanism was recorded incorrectly in the tranche-3 handover brief, and the ledger entry corrects it. The brief stated that
buildDocumentSummaryResultssetssimilarity_origin. It does not. Verified onmainatd0276718:buildDocumentSummaryResultsinsrc/lib/rag/rag-row-contracts.tsstampssimilarity: 1— a fabricated score, not a measured cosine — and sets no origin tag at all. The three call sites that do tag synthetic scores are inrag-candidate-sources.ts(lines 626, 721, 934).deriveConfidenceinsrc/lib/rag/rag-answer-support.ts:32-36buildsstrongestNonSyntheticby excluding rows tagged"synthetic_text", so it is the absence of the tag here that lets the fabricated1.0through, and line 36 then gates"high"onstrongestNonSynthetic >= 0.82with at least two accepted citations. The distinction matters for whoever picks the item up: the fix is adding a tag, not changing one.docs/clinical-hazard-analysis.mdH5a records the adjacent document-lookup fast-path hazard.