feat(rag): remediate clinical retrieval and safety#553
Conversation
# Conflicts: # .env.example # docs/privacy-impact-assessment.md # docs/site-map.md # scripts/compare-retrieval-eval.ts # scripts/eval-quality.ts # src/app/api/documents/[id]/reindex/route.ts # src/app/api/documents/bulk/reindex/route.ts # src/app/api/upload/route.ts # src/app/privacy/page.tsx # src/components/mode-home-template.tsx # src/lib/answer-render-policy.ts # src/lib/answer-verification.ts # src/lib/env.ts # src/lib/rag-cache.ts # src/lib/rag-extractive-answer.ts # src/lib/rag.ts # supabase/drift-manifest.json # supabase/schema.sql # tests/answer-render-policy.test.ts # tests/rag-answer-fallback.test.ts # tests/rag-offline-answer.test.ts
# Conflicts: # docs/privacy-impact-assessment.md # src/lib/answer-render-policy.ts
|
Updates to Preview Branch (codex/rag-remediation) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds access-scoped retrieval, versioned RPC fallbacks, deterministic comparison and claim-support enforcement, producer-scoped deep-memory commits, upload admission controls, privacy notices, and expanded retrieval/offline evaluation metrics with corresponding tests. ChangesRAG platform and governance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: a3eacb22b4
ℹ️ 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 fix all comments and failing CI tests |
|
Summary
Git / PR
Testing
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/ui-smoke.spec.ts (1)
750-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
openUploadSurfacelost the click-retry resilience of the oldopenUploadDrawerhelper.The click at line 759 fires once with no retry;
.poll()(761-769) only re-checks visibility afterward and doesn't re-click if the first click didn't register. The old helper wrapped click+visibility intoPass({ timeout: 8_000 }), retrying the whole action. This is a step back in flake-resistance for a helper used across multiple new smoke tests gating the UI check.As per path instructions, "When fixing review findings, make the smallest clear, scoped, low-risk, testable change."♻️ Proposed fix to restore click-retry resilience
async function openUploadSurface(page: Page) { const uploadButton = page.getByRole("button", { name: /Upload document/i }); const uploadDialog = page.getByRole("dialog", { name: "Upload and indexing" }); const inlineUploadPanel = page.getByRole("tabpanel", { name: "Upload", exact: true }); const visibleUploadTab = page .locator('[role="tab"]:visible') .filter({ hasText: /^Upload\b/ }) .first(); await expect(uploadButton).toBeVisible(); - await uploadButton.click(); - - await expect - .poll(async () => { - return ( - (await uploadDialog.isVisible().catch(() => false)) || - (await inlineUploadPanel.isVisible().catch(() => false)) || - (await visibleUploadTab.isVisible().catch(() => false)) - ); - }) - .toBe(true); + await expect(async () => { + if ( + (await uploadDialog.isVisible().catch(() => false)) || + (await inlineUploadPanel.isVisible().catch(() => false)) || + (await visibleUploadTab.isVisible().catch(() => false)) + ) { + return; + } + await uploadButton.click(); + await expect + .poll(async () => + (await uploadDialog.isVisible().catch(() => false)) || + (await inlineUploadPanel.isVisible().catch(() => false)) || + (await visibleUploadTab.isVisible().catch(() => false)), + ) + .toBe(true); + }).toPass({ timeout: 8_000 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui-smoke.spec.ts` around lines 750 - 782, Restore click-retry behavior in openUploadSurface by retrying the uploadButton click together with the dialog/panel visibility assertion, using the existing bounded retry pattern from openUploadDrawer. Ensure each retry performs a fresh click before checking all supported upload surfaces, while preserving the current dialog and inline-panel selection logic.Source: Path instructions
src/app/api/upload/route.ts (1)
109-141: 🩺 Stability & Availability | 🔵 TrivialFail-closed rate limiting removes upload availability fallback.
allowInMemoryFallbackOnUnavailable: falsemeans an outage in the rate-limit backing store will now hard-block all uploads (503/limited) instead of degrading to an in-memory fallback. This is consistent with the PR's fail-closed containment goal, but it's a meaningful availability trade-off worth explicit monitoring/alerting on the rate-limit store's health so an outage there doesn't silently become "uploads are down."The admission-lease acquisition/release pairing (
releaseAdmissionset right afteracquireUploadAdmission, unconditionally released infinally) is correctly leak-proof across all the newassertUploadNotAbortedthrow points.Also applies to: 362-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/upload/route.ts` around lines 109 - 141, Keep the fail-closed rate-limit configuration in the upload handler by retaining allowInMemoryFallbackOnUnavailable: false in consumeSubjectApiRateLimit, and add explicit monitoring or alerting for rate-limit backing-store health and failures so an outage causing upload unavailability is visible. Preserve the existing acquireUploadAdmission/releaseAdmission pairing and abort checks.src/lib/upload-admission.ts (1)
5-8: 🩺 Stability & Availability | 🔵 TrivialPer-process admission state won't enforce limits across instances.
The controller keeps
active/bytesonglobalThis, so concurrency and byte ceilings are enforced per Node process only. Under serverless/multi-instance deployment each instance admits up to the full limit independently, so aggregate load can exceedmaxConcurrent/maxBytes. If this is intended as a best-effort per-instance shed, a brief comment stating that assumption would help; a shared store (e.g. Redis/DB counter) is needed for a global guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/upload-admission.ts` around lines 5 - 8, Clarify the scope of the state used by the upload admission controller around AdmissionState and the globalThis-backed state: document that active/byte limits are best-effort and enforced per Node process or instance. If global enforcement is required, replace this in-memory global state with atomic shared-store counters, preserving the maxConcurrent and maxBytes admission semantics across instances.supabase/migrations/20260713010000_clear_historical_rag_query_answers.sql (1)
3-5: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider batching this backfill for large tables.
The single
UPDATE ... WHERE answer is not nullrewrites every affected row in one transaction, which can hold locks and bloat WAL on a largerag_queriestable. For the reviewed migration run, consider batching by primary key (e.g. loop overLIMIT-sized id windows) to keep transactions short. TheWHERE answer is not nullguard is good — it already skips already-cleared rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260713010000_clear_historical_rag_query_answers.sql` around lines 3 - 5, Batch the historical update on public.rag_queries instead of rewriting all non-null answers in one transaction. Iterate through primary-key windows using a bounded batch size, updating only rows where answer is not null, and commit each batch while preserving the existing guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/privacy-impact-assessment.md`:
- Around line 45-53: The PIA-3 status terminology is inconsistent across the
document. Standardize all PIA-3 references in the gap table, gap-register
header, Recommendation, and surrounding discussion on one status term,
preferably “Mitigated,” without changing the underlying assessment.
In `@src/lib/rag-claim-support.ts`:
- Around line 265-273: Update isBoundedEvidenceGapSegment so that, after
excluding segments containing clinical value atoms or matching
clinicalRecommendationPattern, non-claim segments return true instead of falling
through to false. Preserve the existing true results for the exact normalized
“no evidence found” matches and the false result for clinical content.
In `@tests/deep-memory.test.ts`:
- Around line 696-737: Update the delete mock in the test’s supabase.from
implementation to record each delete operation in operations, including its
table name and staged/non-staged scope as needed by the existing assertion. Keep
the current cleanup assertion unchanged so unintended non-staged deletes are
detected rather than passing vacuously.
In `@tests/retrieval-access-scope.test.ts`:
- Around line 77-92: Update the mock RPC branch for
"match_document_chunks_hybrid_v2" in the test to return Supabase’s
missing-function response shape: data: null with an error containing the
PGRST202 code. Keep the fallback RPC response and expected ordering unchanged so
callVersionedRetrievalRpc exercises the real fallback path.
---
Nitpick comments:
In `@src/app/api/upload/route.ts`:
- Around line 109-141: Keep the fail-closed rate-limit configuration in the
upload handler by retaining allowInMemoryFallbackOnUnavailable: false in
consumeSubjectApiRateLimit, and add explicit monitoring or alerting for
rate-limit backing-store health and failures so an outage causing upload
unavailability is visible. Preserve the existing
acquireUploadAdmission/releaseAdmission pairing and abort checks.
In `@src/lib/upload-admission.ts`:
- Around line 5-8: Clarify the scope of the state used by the upload admission
controller around AdmissionState and the globalThis-backed state: document that
active/byte limits are best-effort and enforced per Node process or instance. If
global enforcement is required, replace this in-memory global state with atomic
shared-store counters, preserving the maxConcurrent and maxBytes admission
semantics across instances.
In `@supabase/migrations/20260713010000_clear_historical_rag_query_answers.sql`:
- Around line 3-5: Batch the historical update on public.rag_queries instead of
rewriting all non-null answers in one transaction. Iterate through primary-key
windows using a bounded batch size, updating only rows where answer is not null,
and commit each batch while preserving the existing guard.
In `@tests/ui-smoke.spec.ts`:
- Around line 750-782: Restore click-retry behavior in openUploadSurface by
retrying the uploadButton click together with the dialog/panel visibility
assertion, using the existing bounded retry pattern from openUploadDrawer.
Ensure each retry performs a fresh click before checking all supported upload
surfaces, while preserving the current dialog and inline-panel selection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8530dbb-83d0-4991-b526-a3a833101324
📒 Files selected for processing (82)
.env.exampledocs/privacy-impact-assessment.mddocs/site-map.mdscripts/compare-retrieval-eval.tsscripts/eval-quality.tsscripts/eval-rag-offline.mjsscripts/eval-retrieval.tsscripts/fixtures/rag-offline-contract-tests.jsonscripts/generate-site-map.tsscripts/rag-offline-contract.mjssrc/app/api/answer/route.tssrc/app/api/answer/stream/route.tssrc/app/api/search/route.tssrc/app/api/upload/route.tssrc/app/globals.csssrc/app/privacy/page.tsxsrc/components/AccessibleTable.tsxsrc/components/clinical-dashboard/DocumentManagerPanel.tsxsrc/components/clinical-dashboard/answer-result-surface.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/clinical-dashboard/visual-evidence.tsxsrc/components/privacy-input-notice.tsxsrc/lib/answer-render-policy.tssrc/lib/answer-verification.tssrc/lib/citations.tssrc/lib/clinical-search.tssrc/lib/corpus-grounding.tssrc/lib/cross-document-synthesis.tssrc/lib/deep-memory.tssrc/lib/demo-data.tssrc/lib/document-enrichment.tssrc/lib/env.tssrc/lib/evidence.tssrc/lib/owner-scope.tssrc/lib/rag-cache.tssrc/lib/rag-claim-support.tssrc/lib/rag-comparison.tssrc/lib/rag-contracts.tssrc/lib/rag-extractive-answer.tssrc/lib/rag-quote-verification.tssrc/lib/rag-retrieval-variants.tssrc/lib/rag-routing.tssrc/lib/rag.tssrc/lib/retrieval-rpc-rollout.tssrc/lib/search-scope.tssrc/lib/supabase/database.types.tssrc/lib/types.tssrc/lib/upload-admission.tssupabase/drift-manifest.jsonsupabase/migrations/20260713010000_clear_historical_rag_query_answers.sqlsupabase/migrations/20260713020000_owner_plus_public_retrieval.sqlsupabase/migrations/20260713030000_producer_scoped_deep_memory.sqlsupabase/schema.sqltests/answer-render-policy.test.tstests/answer-verification.test.tstests/canonical-answer-table.test.tstests/clinical-search.test.tstests/corpus-grounding.test.tstests/cross-document-synthesis.test.tstests/deep-memory-transaction-sql.test.tstests/deep-memory.test.tstests/document-enrichment-retrieval-scope.test.tstests/eval-quality.test.tstests/eval-retrieval.test.tstests/owner-scope.test.tstests/privacy-ui.test.tstests/privacy.test.tstests/private-access-routes.test.tstests/private-rag-access.test.tstests/rag-answer-fallback.test.tstests/rag-claim-support.test.tstests/rag-comparison.test.tstests/rag-offline-answer.test.tstests/rag-offline-contract.test.tstests/rag-routing.test.tstests/retrieval-access-scope.test.tstests/retrieval-hydration-scope.test.tstests/retrieval-owner-filter-guard.test.tstests/retrieval-rpc-rollout.test.tstests/search-scope.test.tstests/ui-smoke.spec.tstests/upload-admission.test.ts
|
Final review disposition:
Local validation: verify:cheap passed (207 files, 1905 tests; one file/test skipped), focused review suites passed, TypeScript passed, and scratch Postgres schema replay passed. |
All actionable findings were fixed, tested, replied to, and their review threads resolved. Remaining review-body nitpicks were either implemented or explicitly dispositioned in the PR.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/privacy-impact-assessment.md (1)
439-441: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not overstate automated purge coverage.
This paragraph calls automated purge part of the existing strong posture, while the assessment documents that
rag_query_misseshas no purge and expiredrag_response_cacherows can persist until overwrite or manual invalidation. Reword this claim to avoid implying complete retention coverage.Suggested wording
- The data-at-rest security posture (Sydney residency, RLS, private storage, query hashing, automated purge) is already strong and should be highlighted + The data-at-rest controls are strong for covered tables (Sydney residency, RLS, private storage, query hashing, and existing purge jobs), but retention gaps remain for `rag_query_misses` and expired `rag_response_cache` rows🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/privacy-impact-assessment.md` around lines 439 - 441, Revise the PIA-5 residual data-handling documentation to avoid presenting automated purge as comprehensive retention coverage. Acknowledge that rag_query_misses lacks purge and expired rag_response_cache rows may persist until overwrite or manual invalidation, while retaining the other documented security controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/privacy-impact-assessment.md`:
- Around line 439-441: Revise the PIA-5 residual data-handling documentation to
avoid presenting automated purge as comprehensive retention coverage.
Acknowledge that rag_query_misses lacks purge and expired rag_response_cache
rows may persist until overwrite or manual invalidation, while retaining the
other documented security controls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 004d458e-956d-4ff7-8338-00c762f177e2
📒 Files selected for processing (16)
docs/privacy-impact-assessment.mdsrc/components/clinical-dashboard/master-search-header.tsxsrc/lib/deep-memory.tssrc/lib/rag-cache-utils.tssrc/lib/rag-claim-support.tssrc/lib/upload-admission.tssupabase/drift-manifest.jsonsupabase/migrations/20260713020000_owner_plus_public_retrieval.sqlsupabase/migrations/20260713030000_producer_scoped_deep_memory.sqlsupabase/schema.sqltests/deep-memory-transaction-sql.test.tstests/deep-memory.test.tstests/rag-cache-utils.test.tstests/rag-claim-support.test.tstests/retrieval-access-scope.test.tstests/ui-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/deep-memory-transaction-sql.test.ts
- src/lib/upload-admission.ts
- src/components/clinical-dashboard/master-search-header.tsx
- supabase/migrations/20260713030000_producer_scoped_deep_memory.sql
- src/lib/rag-claim-support.ts
- tests/retrieval-access-scope.test.ts
- supabase/drift-manifest.json
- tests/deep-memory.test.ts
- tests/ui-smoke.spec.ts
- tests/rag-claim-support.test.ts
- supabase/schema.sql
- src/lib/deep-memory.ts
- supabase/migrations/20260713020000_owner_plus_public_retrieval.sql
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f60686d13
ℹ️ 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".
| const evidenceDimensions = safetyDimensions(evidence); | ||
| return [...safetyDimensions(claim)].every((dimension) => evidenceDimensions.has(dimension)); |
There was a problem hiding this comment.
When a high-risk safety claim uses wording outside actionPolarity() (for example, Drug A is safe in pregnancy) this check only verifies that the cited source mentions the same safety dimension. A source saying Drug A is unsafe in pregnancy has the same pregnancy dimension, no clinical value atom, and enough topic overlap, so sourceSupportsClaim() can mark the opposite claim as directly supported and the high-risk evidence-gap gate will not fire. Add directional safety-state handling (safe/unsafe, risk/no risk, required/not required, etc.) before treating same-dimension evidence as compatible.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
npm run verify:pr-local— passed before the final one-commitmainsync: 1,890 tests, production build (634 pages), client secret scan, offline RAG 263/263npm run verify:ui— full aggregate was not rerun; targeted changed-surface Chromium checks passed 12/12 during implementationnpm run verify:release— not run; provider/release workflows remain approval-gatednpm run eval:rag:offline— latest exact merge commit: 21 files, 264/264npm run eval:retrieval:quality— not run; requires live/provider credentialsnpm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only— not run; provider-backednpm run check:production-readiness— static production-like readiness passed during implementation with nonfunctional placeholders and no provider callsnpm run check:deployment-readiness— no deployment requestednpm run verify:cheap— latest exact merge commit: 207 files passed, 1 skipped; 1,901 tests passed, 1 skippednpm run drift:manifest— scratch Postgres schema replay passed; live drift was not contactedClinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
/privacycontent and point-of-entry notices are explicitly draft and governance-review-required.