Fix lithium answer recovery and Australian source fallback#607
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR adds source-authority governance and locality tooling, prioritizes Australian clinical context, hardens generation fallback and caching, and standardizes answer-progress SSE events with a staged dashboard UI and expanded evaluation and integration tests. ChangesAnswer governance and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SearchResults
participant RAG
participant AnswerStream
participant ClinicalDashboard
SearchResults->>RAG: rank and select Australian context
RAG->>AnswerStream: emit progress and answer result
AnswerStream->>ClinicalDashboard: send public progress and final SSE events
ClinicalDashboard->>ClinicalDashboard: render timed progress stepper
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/rag.ts (1)
4866-4872: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer fallback-only artifact generation until the catch path.
generationFallbackArtifactsandgenerationFallbackSelectionSummaryare only read inside thecatchblock, but they’re computed on every request. Moving them into the fallback path would avoid the quote extraction, breakdown, coverage, and score-explanation work on successful generations.🤖 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/rag.ts` around lines 4866 - 4872, Move the computations of generationFallbackArtifacts and generationFallbackSelectionSummary from the main request path into the catch-based fallback path where they are consumed. Keep generationFallbackResults available to that path, and preserve the existing fallback artifact and selection-summary behavior while avoiding this work for successful generations.scripts/backfill-source-metadata.ts (2)
684-692: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAudit summary logs unbounded document arrays.
authorityAuditembeds fullconflicts,missing_australian_locality, andproposed_locality_correctionsarrays (one entry per flagged document, with title/file_name/etc.) directly into the console-logged summary, with no sampling. The legacy dry-run path below caps its sample at 20 items; this locality-only path — which runs across the whole corpus — has no equivalent cap and can produce a very large log dump as the document count grows.♻️ Suggested fix: cap the emitted arrays
const summary = { mode: args.apply ? "apply" : "dry-run", scope: "locality-only", allowed_metadata_keys: ["publisher_code", "publisher", "jurisdiction"], documents_seen: documents.length, documents_with_changes: changed.length, - ...authorityAudit, + ...authorityAudit, + conflicts: authorityAudit.conflicts.slice(0, 20), + missing_australian_locality: authorityAudit.missing_australian_locality.slice(0, 20), + proposed_locality_corrections: authorityAudit.proposed_locality_corrections.slice(0, 20), };🤖 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 `@scripts/backfill-source-metadata.ts` around lines 684 - 692, Cap the document-level arrays emitted through authorityAudit in the summary logged by the locality-only backfill path. Apply the existing 20-item sampling convention used by the legacy dry-run path to conflicts, missing_australian_locality, and proposed_locality_corrections while preserving the aggregate counts and other summary fields.
676-763: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the gated write path (
runLocalityOnlyBackfill/main).Existing tests only cover
parseBackfillSourceMetadataArgsand static source-content assertions on the script; the actual--apply --confirmwrite flow (RPC call, patch validation, per-document error handling) is untested. This is exactly the class of database-write code the guidelines call out for careful regression/rollback-safety review.Consider extracting a thin, injectable
supabase(already parameterized) and adding a unit test that mocksrpc("apply_document_metadata_patch", ...)to confirm: patches are only sent for!unresolvedConflictdocuments,assertLocalityMetadataPatchrejects disallowed keys before any RPC call, and a single RPC failure surfaces a clear error without silently continuing.As per coding guidelines, "For high-risk code involving authentication, authorization, private data, database access, clinical output, ingestion, provider calls, billing, uploads, or background jobs, review carefully for regressions, missing tests, rollback safety, and conservative failure behavior."
🤖 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 `@scripts/backfill-source-metadata.ts` around lines 676 - 763, Add unit coverage for the apply path in runLocalityOnlyBackfill, using its injected supabase client and invoking it through the --apply --confirm flow in main where appropriate. Mock rpc("apply_document_metadata_patch", ...) to verify only non-conflicting documents produce validated patches, disallowed metadata keys are rejected before any RPC call, and the first RPC error is surfaced with the document context without continuing.Source: Coding guidelines
🤖 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 `@scripts/eval-rag.ts`:
- Around line 237-251: Reset or recreate the progress collection inside each
callback attempt passed to withProviderBackoff, so buildRagEvaluationDiagnostics
receives only events from the attempt that produced the final answer. Update the
surrounding answerQuestionWithScope flow while preserving the existing progress
event handling and diagnostics construction.
---
Nitpick comments:
In `@scripts/backfill-source-metadata.ts`:
- Around line 684-692: Cap the document-level arrays emitted through
authorityAudit in the summary logged by the locality-only backfill path. Apply
the existing 20-item sampling convention used by the legacy dry-run path to
conflicts, missing_australian_locality, and proposed_locality_corrections while
preserving the aggregate counts and other summary fields.
- Around line 676-763: Add unit coverage for the apply path in
runLocalityOnlyBackfill, using its injected supabase client and invoking it
through the --apply --confirm flow in main where appropriate. Mock
rpc("apply_document_metadata_patch", ...) to verify only non-conflicting
documents produce validated patches, disallowed metadata keys are rejected
before any RPC call, and the first RPC error is surfaced with the document
context without continuing.
In `@src/lib/rag.ts`:
- Around line 4866-4872: Move the computations of generationFallbackArtifacts
and generationFallbackSelectionSummary from the main request path into the
catch-based fallback path where they are consumed. Keep
generationFallbackResults available to that path, and preserve the existing
fallback artifact and selection-summary behavior while avoiding this work for
successful generations.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 06b7b2e3-3d0e-4d79-a251-5597a38a1b1b
📒 Files selected for processing (24)
docs/branch-review-ledger.mdscripts/audit-source-governance.tsscripts/backfill-source-metadata.tsscripts/eval-rag.tssrc/app/api/answer/stream/route.tssrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/answer-progress.tssrc/components/clinical-dashboard/answer-status.tsxsrc/lib/answer-progress-public.tssrc/lib/australian-source-priority.tssrc/lib/rag-context-selection.tssrc/lib/rag-eval-diagnostics.tssrc/lib/rag.tssrc/lib/source-authority-metadata.tssrc/lib/source-authority-registry.tssrc/lib/source-metadata.tstests/answer-progress-ui-smoke.spec.tstests/answer-progress.test.tstests/private-access-routes.test.tstests/rag-answer-fallback.test.tstests/rag-context-budget.test.tstests/rag-eval-source-governance.test.tstests/source-authority-tooling.test.tstests/source-metadata.test.ts
Keep eval progress scoped to the successful provider attempt, bound locality audit output, and cover the metadata apply path. Verified with focused tests, verify:cheap, and production readiness.
Keep deterministic dose and threshold recovery scoped to evidence cited by the final answer, including after the updated claim-scoped numeric verifier from main.
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Heuristic only — a main-side or flake label is a starting point, not a verdict. |
When generation fails, keep deterministic dose and threshold recovery scoped to one complete evidence chunk instead of stitching figures across sources.
…recovery-pr # Conflicts: # src/app/api/answer/stream/route.ts
…' into codex/lithium-answer-recovery-pr # Conflicts: # src/lib/rag.ts
![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: 43a385d207
ℹ️ 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. |
|
To use Codex here, create a Codex account and connect to github. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Keep the filtered fallback candidate at the RagAnswer boundary so current TypeScript inference accepts the single-source recovery assignment.
…' into codex/lithium-answer-recovery-pr # Conflicts: # src/lib/rag.ts
1 participant
Summary
Lithium dosingVerification
npm run verify:pr-localnpm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:releasebefore release or handoff confidence claimsnpm run eval:retrieval:quality(must stay 36/36) when retrieval, ranking, selection, chunking, or scoring behavior changednpm run eval:rag -- --limit 15+npm run eval:quality -- --rag-onlywhen answer generation, the synthesis prompt, or answer post-processing changedLithium dosingevaluation passed with a grounded FSH citation, no generic failure, no invalid citations, no unverified numeric tokens, no authority conflict, and no threshold failures; generation timed out and the new source-backed fallback completednpm run check:production-readinesswhen clinical workflow, privacy, environment, Supabase, source governance, or deployment behavior changednpm run check:deployment-readinesswhen deployment startup, hosting, or rollout behavior changedClinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
origin/maincompleted cleanly. This PR does not deploy or merge anything.Summary by CodeRabbit
New Features
Bug Fixes