fix(rag): scope query-term corrector to public titles (F10)#697
Conversation
…F10) correct_clinical_query_terms() is SECURITY DEFINER and bypasses RLS. It built its spell-correction vocabulary from every indexed document title regardless of owner, while the rest of retrieval is strictly owner-scoped and fail-closed (retrieval_owner_matches, docs/tenancy-defense-in-depth-review.md). The unscoped scan folded private tenants' title tokens into every caller's correction vocabulary — a cross-tenant existence side-channel for private title tokens (low severity: token existence, not content). Ship the fix as forward migration 20260717120000_corrector_public_titles_only.sql that recreates the function with the title union restricted to the public corpus (owner_id is null). RAG aliases are curated/global and unchanged; the signature is unchanged, so callers and generated types need no update. Adds migration-backed tests (public-only scope, regression guard against the old unscoped predicate, and service_role execute confinement). Migration-first on purpose: syncing supabase/schema.sql and regenerating supabase/drift-manifest.json need the Docker replay (npm run drift:manifest) that CI/the agent sandbox cannot run, so those are coupled to the live-apply step and performed together by an operator. The migration is NOT applied to the live project here (confirmation-gated); until applied, live == schema.sql == manifest, so merging this file introduces no drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJpsbpsmjJ2tLLXFfSe9GK
|
Updates to Preview Branch (claude/audit-findings-review-phgz92) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a migration that recreates the clinical query-term corrector using indexed public document titles and public aliases, preserves similarity-based token correction, and limits execution to ChangesPublic clinical query corrector
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 066a0ea52c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c286bd1c54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow-up to the corrector title scoping: the two rag_aliases reads in correct_clinical_query_terms were still unscoped. rag_aliases carries an owner_id (there is a rag_aliases_owner_enabled_idx), and deep-memory.ts persists owner-scoped aliases/canonicals when a private document is indexed (src/lib/deep-memory.ts). Because the function is SECURITY DEFINER and bypasses RLS, a caller in another tenant could still trigger and observe corrections toward private document-derived alias/canonical terms — the same cross-tenant side-channel the title filter closed, via aliases. Scope both alias selects to the public corpus (owner_id is null), matching the title scan and the public-titles-only decision. Extends the migration-backed test to assert the scoped alias reads and guard against the old unscoped form. Still migration-only (schema.sql/manifest sync coupled to the Docker-gated live-apply); schema.sql remains untouched so the drift manifest stays fresh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJpsbpsmjJ2tLLXFfSe9GK
…manifest (F10) Completes the F10 corrector tenancy fix. Migration 20260717120000_corrector_public_titles_only (merged in #697) scoped the SECURITY DEFINER query-term corrector's vocabulary — both rag_aliases reads and the document-title scan — to the public corpus (owner_id is null). This mirrors that same predicate into the canonical supabase/schema.sql corrector definition so a disaster-recovery restore or manifest/schema reconciliation cannot recreate the unscoped, cross-tenant vocabulary (Codex review P1 on #697). Regenerated supabase/drift-manifest.json via `npm run drift:manifest` (real Docker Postgres replay): schema_sha256 and only the corrector function's def_hash change (8f1094c4… → a5ebdd1e…), confirming the sole schema delta is the scoped corrector. Offline drift-detection + schema tests pass (66). Live apply of the migration to the Clinical KB Database project remains a separate, confirmation-gated deploy-pipeline step (needs service-role env); after applying, `npm run check:drift` should report clean against live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJpsbpsmjJ2tLLXFfSe9GK
2 participants
Summary
Remediates F10 from the RAG-scalability review handover: the clinical query-term corrector leaked private title tokens across tenants.
public.correct_clinical_query_termsisSECURITY DEFINER(bypasses RLS) and runs on the live retrieval path (src/lib/rag-candidate-sources.ts,src/lib/rag.ts). It built its spell-correction vocabulary from every indexed document title regardless of owner, while the rest of retrieval is strictly owner-scoped and fail-closed (retrieval_owner_matches,docs/tenancy-defense-in-depth-review.md). The unscoped scan folded private tenants' title tokens into every caller's correction vocabulary — a cross-tenant existence side-channel for private title tokens (severity P2 / low: token existence, not content).Fix — public titles only:
RAG aliases are curated/global and unchanged; the function signature is unchanged, so callers and generated types need no update.
Why migration-only (important)
The fix ships as forward migration
20260717120000_corrector_public_titles_only.sqlplus migration-backed tests, and deliberately does not editsupabase/schema.sqlin this PR:schema.sqlrequires regeneratingsupabase/drift-manifest.json, which is a Docker-Postgres replay artifact (npm run drift:manifest) — Docker isn't available in the CI/agent sandbox, andtests/drift-detection.test.tsfails offline on a stale manifest.schema.sqlmirror + manifest regen are therefore coupled to the live-apply step and performed together by an operator in a Docker-capable environment.live == schema.sql == manifest(all the pre-scope version), so merging this migration file introduces no drift —db-reset-verifysimply replays it into a scratch DB.Verification
npx vitest run tests/drift-detection.test.ts tests/supabase-schema.test.ts— 65 passed (drift manifest still fresh sinceschema.sqlis untouched; 2 new corrector tests: public-only scope, regression guard,service_roleconfinement)npx prettier --checkon the changed test — cleannpm run verify:pr-local— not run (nosrc/UI surface changed)npm run verify:ui— not applicableClinical Governance Preflight
Touches retrieval + document access, so completed:
Clinical KB Database(sjrfecxgysukkwxsowpy)SECURITY DEFINERwithexecuteconfined toservice_role(asserted in tests)Operator follow-up (Docker + confirmation-gated)
supabase/schema.sql, thennpm run drift:manifestto refresh the drift manifest — do these together.npm run check:supabase-projectafter applying.No production mutations, no Supabase apply, no OpenAI/provider calls were performed in this PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FJpsbpsmjJ2tLLXFfSe9GK
Generated by Claude Code
Summary by CodeRabbit