fix(db): restrict owner deletion on the four null-owner-means-public tables (#ZBAC9D) — AWAITING DEPLOY WINDOW - #2502
Conversation
…tables (#ZBAC9D) A null owner_id independently means "public corpus" to retrieval, and these owner foreign keys were `on delete set null`, so deleting an auth user converted that user's private rows into public ones silently. Fix the foreign key, not the predicate. retrieval_owner_matches and its v2 are only unsafe because the FK can manufacture null owners; make that impossible and "null owner = deliberately published" holds by construction. No query result changes, so there is no retrieval behaviour change and no eval canary is required. Scope is the four tables whose OWN owner_id reaches a retrieval owner predicate: documents (25 call sites across both predicates), document_labels, document_summaries and document_table_facts. The dispatch brief named six; document_sections and document_embedding_fields are filtered through their parent document's owner, never their own, and were dropped after resolving every call site. Retention tables keep `set null` deliberately. The migration proves its own effect via pg_constraint.confdeltype rather than trusting recorded history, and the new guard test pins the restrict set to exactly those four so neither narrowing nor widening passes unnoticed. Migration and schema.sql move together; the regenerated drift manifest is still owed and needs Docker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the migration/mirror/manifest trio. The replay moved exactly the four owner foreign-key constraint entries from ON DELETE SET NULL to ON DELETE RESTRICT, plus generated_at and schema_sha256; every other object across 207 constraints, 96 functions, 43 tables, 222 indexes, 53 policies and 26 triggers hashed identically. The replay also independently proves the edited schema.sql is valid SQL: it is applied from scratch into the pinned supabase/postgres container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a885e47c-cf67-4c16-9043-387e4741a78a) |
|
Updates to Preview Branch (claude/zbac9d-owner-fk-restrict) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
…annot defer * fix(pr-policy): block migration PRs that promise a deploy this repo cannot defer Merging a PR that touches `supabase/migrations/**` applies it to the live clinical database within seconds — the Supabase integration has "Deploy to production" enabled on `main`. There is no separate deploy step, so metadata claiming the change is held back describes a control that does not exist, and it is dangerous exactly when believed: it invites a reviewer to merge something they think is still parked. PR #2502 merged carrying "AWAITING DEPLOY WINDOW" in its own title and reached the live database within minutes; the post-merge live-drift run caught it as pending-apply drift and a manual re-run cleared it. No harm that time — the migration was protective — but the phrasing would read identically on a change that genuinely should not go live yet. `evaluatePullRequestPolicy` gains a third blocking gate beside the clinical governance and RAG-impact ones: a diff carrying a Supabase migration must not claim a deferred deploy. The error quotes the offending phrase back so the author does not have to guess which words tripped it. The pattern list is deliberately narrow. Choosing WHEN TO MERGE is the real control and AGENTS.md requires it, so "merge only inside the approved deploy window" must keep passing; only claims that the deploy is separate from, or later than, the merge are matched. Verification: - `npm run check:pr-policy` — self-test passed; workflow guard passed. - `npm run lint` — clean (fresh run, 5990 input files). - The real PR #2502 title and file list replayed through the gate: blocked, quoting "AWAITING DEPLOY". - Mutation-verified both directions: disabling the gate fails the self-test ("must be blocked"); widening it to match any "deploy window" also fails ("naming an approved merge window must not trip the deferral gate"). - Typecheck not run: tsconfig includes .ts/.tsx/.mts only and checkJs is off, so it cannot cover a .mjs change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015uQ7pGjA4hBxi9TRNsfjAL * fix(pr-policy): close both deferral-gate holes found in review Two confirmed findings on the first commit, both reproduced before fixing. P1 — false negatives. `this migration will be deployed manually after merge` and `deployment is pending operator approval` both returned claimed:false. The first was uncovered entirely; the second failed only because the subject noun phrase sits between "Deployment" and "pending" and the window was 24 characters. Both are natural phrasings of exactly the claim the gate exists to block. Added out-of-band, post-merge and reversed-order patterns, and widened the reversed-order window to clause width. P2 — false positive. `Do not deploy the staging worker until its image passes smoke tests` hard-failed any PR that also carried a migration, though it makes no claim about when the migration applies. The detector now scopes body statements per clause and requires the database subject; the title stays unconditionally in scope, because a title describes the whole PR and this gate only runs where that PR carries a migration. `appl(?:y|ies|ied|ication)` replaces the bare `appl` prefix so "pending clinical governance approval" — an approval sought BEFORE merge, which is the sanctioned control — is no longer read as a deferred apply. A ten-row behaviour table is pinned in the self-test. The five false rows are each a statement the repository requires authors to be able to make, including the operator-prebuild index workflow and the truthful "applies to the live database within seconds of merge" wording the gate steers toward. Verification: - `npm run check:pr-policy` — self-test passed; workflow guard passed. - `npm run lint` — clean (fresh run, 5990 input files). - Both findings reproduced first: claimed:false on the two P1 phrasings, claimed:true quoting "Do not deploy" on the P2 sentence. - Mutation-verified: dropping the subject scoping fails on the staging-worker row; removing the reversed-order pattern fails on the operator-approval row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015uQ7pGjA4hBxi9TRNsfjAL --------- Co-authored-by: Claude <noreply@anthropic.com>
Important
DO NOT MERGE, AND DO NOT ARM AUTO-MERGE, UNTIL THE OWNER OPENS A DEPLOYMENT WINDOW.
This PR contains a migration. The Supabase integration applies migrations merged to
mainto the live clinical database within seconds, so merging this is deploying it. Merge approval is deploy approval.Summary
on delete set nulltoon delete restrict, closing the orphaned-document republication hazard in#ZBAC9D.supabase/schema.sqland regeneratesupabase/drift-manifest.jsonin the same change, so migration, mirror and manifest move together.The defect
A null
owner_idindependently means "public corpus" to retrieval:public.retrieval_owner_matchesandpublic.retrieval_owner_matches_v2both resolve the public sentinel torow_owner_id is nulland check no published marker. Those owner foreign keys wereon delete set null. So deleting an auth user converted that user's private rows into public ones, silently, and the helper written to catch it —src/lib/documents/is-public-document.ts— has no production caller.Measured on production 2026-09-01 by an owner-approved read-only count: 2851 documents, 0 privately owned, all 2851 carrying
metadata.public_corpus = true, so the exposed count is 0. Nothing is leaking; the mechanism is armed with nothing to leak. It must be closed before the first privately-owned document exists, after which it would be live and silent.Why fix the foreign key rather than the predicate
retrieval_owner_matchesis only unsafe because the FK can manufacture null owners. Make that impossible and "null owner = deliberately published" holds by construction. Changing theon deleteaction of a foreign key cannot change the result of any query, so there is no retrieval behaviour change and no eval canary is required.Editing the predicate instead would be worse on every axis: it needs a signature change rippling through roughly 25 call sites across 13 RPCs and both predicate versions, it is also a migration so it deploys anyway, and it would need a canary.
Scope: four tables, not six
The dispatch brief named six. Resolving every call site brings it to four — the tables whose own
owner_idreaches a retrieval owner predicate:documentsretrieval_owner_matchesand its v2document_labelsdocument_summariesdocument_table_factsdocument_sectionsanddocument_embedding_fieldswere dropped from the brief's list: they are filtered through their parent document's owner, never their own, so a null owner carries no visibility meaning there. They reached the list through an ambiguous table-alias match, where two short aliases each resolve to two tables; reading the actual call sites settles it the other way.document_memory_cardswas checked for the opposite error and is correctly excluded — it too filters on the parent document's owner.The retention tables keep
set nulldeliberately. Foraudit_logs,rag_queries,rag_retrieval_logs,rag_query_misses,rag_answer_feedback,import_batches,storage_cleanup_jobs,rag_visual_eval_cases,document_index_quality,document_index_units,document_sections,document_embedding_fieldsanddocument_memory_cards, nulling the owner on user deletion is intended behaviour that must survive the account being removed.A second flawed predicate was found in passing.
retrieval_owner_matches_v2carries the same "null means public, no marker checked" logic, with aninclude_publicparameter defaulting to true. It does not change the table scope, since all its real call sites are ondocuments, but any future attempt to repair the predicate instead of the foreign key must fix both versions rather than one. Worth recording against#ZBAC9D.Operational consequence, intended
After this lands, deleting an auth user who still owns rows in these four tables will fail rather than silently orphaning them. Any account-deletion flow must reassign or delete that user's documents first. Failing closed is the correct posture for a clinical corpus, but it is a real behaviour change to account deletion and should not be discovered by surprise.
Evidence
Manifest regeneration — the semantic diff, not just the file diff.
npm run drift:manifestmoved exactly four snapshot entries, all four owner foreign keys, fromON DELETE SET NULLtoON DELETE RESTRICT: total snapshot entry changes 8, being four removed and four added, plusgenerated_atandschema_sha256. Every other object across 207 constraints, 96 functions, 43 tables, 222 indexes, 53 policies, 26 triggers, 1 view, 6 extensions and 2 storage buckets hashed identically. That replay also independently proves the editedschema.sqlis valid SQL, since it is applied from scratch into the pinnedsupabase/postgres:17.6.1.127container.The schema mirror edit was verified by enumeration, not by reading the diff. Git's hunk headers name the preceding
create table, which is actively misleading here. Listing theon deleteaction for all 25 owner-bearing tables confirms exactly four arerestrictand every other table is unchanged.Tests.
Covering
supabase-schema,drift-detection,migration-history-guards,migration-history-alignment,hosted-migration-role-guard,function-grants,owner-scope-guard,retrieval-access-scope,retrieval-owner-filter-guard,rag-imputation-contract,search-health-index-coverageandlive-drift-workflow. Thecheck:function-grants: FAILlines in that run's stdout are that suite's own negative fixtures, which feed deliberately-bad SQL to prove the guard fires.Note that running the focused selector over the changed data files finds no tests and exits 0 — Vitest's
relatedselector follows the module import graph and cannot see.sqlor.jsonfiles that are read at runtime. The suites above were discovered by grep and run explicitly rather than trusting that zero.The migration itself is deliberately not replayed locally. CI's
Migration replayjob (db-reset-verify) replays the whole chain on every database-touching PR, so that failure class is already covered by an automatic gate and duplicating it locally would buy nothing. That job going green is a precondition for opening the deployment window.Not run, by design.
npm run check:driftreads the live database and was not run. No Supabase, OpenAI or Railway command was executed. The only live access in this line of work was the earlier owner-approved read-only count.Deployment notes for the window
pg_constraint.confdeltypeand raises if any of the four is notRESTRICT, so it cannot be recorded as applied without having taken effect — the#Q5JHBJ"statements never executed" shape.owner_idondocuments, so validation is expected to be trivial. That was measured fordocumentsonly; if validation fails on one of the other three, that failure is itself a finding, because it means a row references a deleted user.live-driftrun is the gate that confirms the schema actually moved.RAG impact: no retrieval behaviour change — this alters only the ON DELETE action of owner foreign keys, which affects what happens when an auth user is deleted and cannot change the result of any query. No retrieval, ranking or selection code is edited, and no eval canary is required.
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)No clinical decision-support behaviour changes here: retrieval returns exactly what it returned before. The change is to document visibility integrity, and it makes the failure mode more conservative rather than less.
Left for the owner, deliberately not done
src/lib/documents/is-public-document.tsremains dead code. Once the foreign key cannot produce orphans it is genuinely redundant, but it is also the only place expressing "public means published, not merely unowned". Recommendation: keep it and wire it into the retrieval result path as defence in depth, in a separate PR — that would be a retrieval behaviour change needing its own canary, and it should not ride this one. Deleting it is the defensible alternative. It should not be left as it is, tested but unused, because that reads as protection which is not actually there.🤖 Generated with Claude Code
Note
High Risk
Changes auth-user deletion behavior and ownership FKs on clinical document visibility tables, closing a silent private-to-public republication path; account deletion can now fail until owned corpus rows are handled.
Overview
Closes #ZBAC9D by changing
owner_idforeign keys ondocuments,document_labels,document_summaries, anddocument_table_factsfromON DELETE SET NULLtoON DELETE RESTRICT. Previously, deleting an auth user could nullowner_idon those rows; retrieval treats null owner as the public corpus, so private content could become publicly visible without changing query logic.A new migration applies the four constraint swaps with lock/statement timeouts and a post-check on
pg_constraint.confdeltypeso the migration cannot succeed unless all four are RESTRICT.supabase/schema.sqlandsupabase/drift-manifest.jsonare updated to match.tests/supabase-schema.test.tsadds guards that only those four visibility tables use restrict, retention-related tables stay off the restrict set, and the migration content matches expectations.Operational change: user deletion now fails if that user still owns rows in those tables; documents must be reassigned or removed first. Retrieval/RAG query results are unchanged.
Reviewed by Cursor Bugbot for commit 939676c. Configure here.