Improve Clinical KB dashboard and RAG hardening - #11
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:810b1bce1b
ℹ️ 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 fallbackOwnerId = await resolveOwnerFromDocuments(supabase); | ||
| if (fallbackOwnerId) return fallbackOwnerId; |
There was a problem hiding this comment.
When LOCAL_NO_AUTH_OWNER_EMAIL is set but LOCAL_NO_AUTH_OWNER_ID is not, this returns the most recent document owner before ever looking up the configured email. In a shared or stale local database with any existing document owned by someone else, local no-auth private API calls will list, rename, or delete that other owner’s documents instead of the configured user’s; only fall back to documents when no email was configured or the email lookup fails as intended.
Useful? React with 👍 / 👎.
| .single(); | ||
| if (updateError) throw new Error(updateError.message); | ||
| invalidateRagCachesForOwner(user.id); |
There was a problem hiding this comment.
The public search/answer routes now call RAG with ownerId: undefined, so cached entries are stored under the anonymous scope, but this mutation only invalidates the authenticated owner scope. After a rename or delete, public users can keep receiving the old title or deleted source content until the anonymous search/answer cache TTL expires; invalidate the anonymous/all-document scope as well, or clear all RAG caches on document mutations.
Useful? React with 👍 / 👎.
| const [{ data: images, error: imagesError }, { data: chunks, error: chunksError }] = await Promise.all([ | ||
| supabase.from("document_images").select("storage_path").eq("document_id", id), | ||
| supabase.from("document_chunks").select("id").eq("document_id", id), |
There was a problem hiding this comment.
When deleting a document with more rows than the Supabase API page cap (commonly 1,000), this single select only captures the first page of document_images/document_chunks before the cascading document delete removes the DB rows. Any image storage paths not returned here are then lost and remain orphaned in storage, and query-log references for chunks beyond the cap are not removed; load these rows with ranged pagination before deleting the document.
Useful? React with 👍 / 👎.
| const deepMemory = await upsertDocumentDeepMemory({ | ||
| supabase, | ||
| document, | ||
| chunks: chunksResult.data, | ||
| images: imagesResult.data ?? [], |
There was a problem hiding this comment.
In enrichment-only reindexing, the queries above cap the source data at 1,000 chunks and 200 images, but upsertDocumentDeepMemory deletes the document's existing sections/cards before inserting the new set. For any larger indexed document, clicking enrichment reindex silently drops deep-memory coverage for all omitted chunks/images while leaving the document marked indexed; paginate all chunks/images for the memory rebuild, or avoid rebuilding memory from the sampled enrichment prompt set.
Useful? React with 👍 / 👎.
| .eq("owner_id", user.id) | ||
| .overlaps("source_chunk_ids", chunkIds); |
There was a problem hiding this comment.
Because the search/answer endpoints now log public requests with owner_id: null, this owner filter only deletes authenticated-owner log rows. If a public answer used this document before it is deleted, the rag_queries row (including the answer and source_chunk_ids for the deleted chunks) remains after the document deletion; include null-owner rows for the chunk ids or delete by source_chunk_ids regardless of owner when removing the source.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c8c0b10e1c
ℹ️ 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".
| {pagination?.hasMore ? ( | ||
| <button | ||
| type="button" | ||
| onClick={onLoadMoreDocuments} | ||
| disabled={loadingMoreDocuments} | ||
| className={cn(floatingControl, "w-full justify-center")} | ||
| > | ||
| {loadingMoreDocuments ? <Loader2 className="h-4 w-4 animate-spin" /> : <ChevronDown className="h-4 w-4" />} | ||
| Load more documents | ||
| </button> |
There was a problem hiding this comment.
When a library has more than the first 12 documents, this new pagination control fetches and appends more rows, but the list above still renders only filtered.slice(0, 12). As a result, clicking “Load more documents” can update the count/pagination without making the newly loaded older documents visible or manageable unless the user already knows a search term; render the loaded page size (or remove the fixed slice) when pagination is enabled.
Useful? React with 👍 / 👎.
| while (start > 0 && start < end && /\S/.test(clean[start - 1] ?? "") && /\S/.test(clean[start] ?? "")) { | ||
| start -= 1; | ||
| } |
There was a problem hiding this comment.
For an extracted paragraph with a very long unbroken token or OCR artifact, this loop can walk start all the way back to 0; readableOverlapStart then returns 0, and the caller assigns start = 0 again, so chunkTextBySentence repeats the same first chunk forever and the worker hangs on that document. Keep the word-boundary adjustment from returning a position that is not greater than the previous chunk start, or fall back to end - overlap when no whitespace is found.
Useful? React with 👍 / 👎.
| /\b(?:version|revision)\s+\d+\s*$/i, | ||
| ]; | ||
| const lineNoisePatterns: RegExp[] = [ | ||
| /\b(page|p\.?)\s*\d+\s*(?:\/\s*\d+)?\b/i, |
There was a problem hiding this comment.
Because removePageNoise drops every line matching this unanchored pattern, any real guideline sentence that mentions a page reference, such as instructions to use a form on page 3 or continue a monitoring schedule on page 12, is removed before chunking and embedding. Restrict this to standalone page headers/footers rather than any occurrence of page/p. plus a number inside clinical content.
Useful? React with 👍 / 👎.
| const [deleteConfirmation, setDeleteConfirmation] = useState(""); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [pending, setPending] = useState(false); | ||
| const canManage = !disabled && authStatus === "authenticated"; |
There was a problem hiding this comment.
When NEXT_PUBLIC_LOCAL_NO_AUTH is enabled, the dashboard/viewer pass disabled={false} because private APIs are usable without a bearer token, and the routes resolve the owner via local no-auth. This component then re-checks authStatus === "authenticated", which remains signed_out/unconfigured in that mode, so rename and delete are disabled even though the same requests would succeed; base this gate on the parent disabled/local-no-auth state instead of requiring a Supabase session.
Useful? React with 👍 / 👎.
| currentPort: port, | ||
| projectPortStart, | ||
| projectPortEnd, | ||
| safeLocalOrigin: !local || isManagedProjectPort(port), |
There was a problem hiding this comment.
requireAuthenticatedUser uses this guard before bypassing auth in local no-auth mode, but every non-local request URL is marked safe here. If a development server with LOCAL_NO_AUTH/NEXT_PUBLIC_LOCAL_NO_AUTH is exposed via a LAN address, tunnel, or other non-local hostname while NODE_ENV is not production, remote callers can hit private document APIs without authentication; only managed localhost project URLs should be accepted for the no-auth bypass.
Useful? React with 👍 / 👎.
| "im", | ||
| "po", |
There was a problem hiding this comment.
These two-letter route markers are later checked with containsAny, which uses substring matching against the whole query. Non-dosing queries containing ordinary words like “important”, “improve”, or “support” therefore set hasDosingSignals and receive the medication/dose ranking boost, pushing dose-heavy chunks ahead of more relevant sources; match short route abbreviations with token boundaries instead of substring inclusion.
Useful? React with 👍 / 👎.
| }; | ||
| bucket.count += 1; | ||
| buckets.set(key, bucket); |
There was a problem hiding this comment.
The public search/answer routes insert into these module-level maps for every distinct forwarded IP key, but expired keys are never swept unless that exact key is seen again. In self-hosted or local deployments where callers can vary X-Forwarded-For/X-Real-IP, a low-rate stream of unique values both bypasses the per-IP limit and grows memory indefinitely; add trusted-proxy handling plus max-size/expiry cleanup.
Useful? React with 👍 / 👎.
…se 1) (#218) * fix(rag): harden alias cache on transient failure; document finding #11 deferral fetchEnabledRagAliases no longer caches an empty result on a transient rag_aliases read failure. Caching [] suppressed alias-based query expansion (and could let an alias-rescuable query short-circuit) for the whole TTL; it now returns empty for the failing call only and retries next call. Also documents in docs/process-hardening.md why finding #11 (nondeterministic "unsupported" retrieval — bipolar/anorexia intermittently returning 0 results) has no safe Phase-1 fix: the deterministic classifier cannot distinguish in-corpus from out-of-corpus topics, and removing the LLM-gated soft-tail short-circuit regresses eval unsupported_correct 1.0->0.79 in this broad corpus. Deferred to Phase 2 corpus-grounded relevance (IDF/semantic + RC6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(rag): deterministic ranking tiebreak below the engineered signals finalScore is clamped to [0,1], so heavily-boosted results saturate at 1.0 and tie. rankClinicalResults now falls through to the pre-clamp boost magnitude (recovering the boost engineering the clamp discards) and finally a stable id comparison, so fully-tied results no longer order by arbitrary retrieval order (a residual run-to-run nondeterminism). Both levels fire only after score/tieBreakScore have tied, so ranking quality is unchanged: golden retrieval stays 23/23 with mrr@10=0.7283 identical to baseline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
One small step for each open item from the universal-search workstream
(docs/rag-hybrid-findings-and-todo.md items 17-25):
- Item 17 (alias promotion blocked by redaction): weak-search misses now
store RET-H4-safe candidate aliases — canonical terms from the curated
clinical vocabulary that the query matched (output text comes from the
fixed vocabulary table, never the raw query), via new
queryVocabularyAliasesForStorage. Raw tokens still require
RAG_PERSIST_RAW_QUERY_TEXT.
- Item 19 (demo fallback masks live failures): the shared fallback choke
point (nonProductionSupabaseDemoFallbackReason) now console.warns loudly,
naming the env vars to check; behaviour and headers unchanged.
- Item 20 (governance-weighting guard): verified already covered by the
existing keys-free structural test in retrieval-selection.test.ts —
marked done in the findings doc.
- Item 21 (gate recalibration): synthetic_similarity_count and
text_or_relaxation_used now persist into rag_retrieval_logs.metadata
(they were computed but dropped by the telemetry whitelist), so the
recalibration has data to work from.
- Owner-auth e2e: new universal-search-owner-live.test.ts signs in with the
E2E password user via supabase-js and exercises the real route handler
with a genuine bearer token; skips cleanly when live env is absent
(browser-login coverage is not feasible — header sign-in is
magic-link/OAuth only).
- Cross-mode chips now show live counts ("Forms (2)") from the universal
typeahead response, only when fresh results exist for the exact query.
- Items 18 (index-unit HNSW/ef_search), 22 (registry-to-corpus embedding),
23 (finding #11 Phase 2), 24 (OCR dropped letters), 25 (latency):
upgraded from vague notes to concrete measured/stepped specs in the
findings doc — each needs live keys or major scope, so a spec is the
honest smallest step.
Verified: verify:cheap green (1143 tests; live spec skips without keys),
format:check clean, ui-universal-search.spec.ts 3/3 against a live
demo-mode dev server (npm run ensure now works in this container thanks to
the upstream EAFNOSUPPORT fix).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QiyE8jMm7VnrtknHF6jnJ#1046) Against current main (post-#1033, which already resolved#2/#3): - Capture the still-untracked auth DB-connection allocation debt as #11: operator-only Supabase dashboard action (percentage-based allocation before compute scale-up), not settable via SQL/MCP. next-id -> 012. - Fix#5 Source path src/lib/rag/clinical-search.ts:1362 -> src/lib/clinical-search.ts:1735 (the rag/ path does not exist; it was failing docs:check-links on main) and note ordering already sorts by the unbounded pre-clamp rankScore, so the clamp bounds only reported confidence. Docs-only. No code/protected-surface edits, no provider/CI/dashboard action. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the docs/outstanding-issues.md conflict: main added #11 (auth DB-connection allocation) via its own ledger update, so this renumbers the site-audit follow-ups from #11-#16 to #12-#17 (next-id -> 018) and fixes the #17 cross-reference. Keeps main's #11 intact; no other ledger rows changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UbhUVWVJRwDibC2YtJ6aRX
analyzeQueryWithClassifierFallback memoizes every classifier verdict — accepted or rejected — for 15 minutes so the same query gets a consistent result within a session (Finding #11 interim fix). For the soft-tail bucket (the same fragile, low-confidence case the unsupported short-circuit treats specially), that made a rejected verdict sticky for 15 minutes, 15x longer than the 60s search-cache TTL the previous commit stopped writing to — so that fix alone did not meaningfully unstick a repeat "catatonia"-style query. A rejected verdict for that specific bucket is no longer memoized, so a repeat query gets a fresh classifier attempt instead of reproducing the same rejection for the rest of the TTL window. Accepted verdicts, and rejected verdicts outside the soft-tail bucket, keep the existing determinism guarantee unchanged. Addresses a P1 finding from automated PR review on #1646. RAG impact: this changes how often the *same* query can get a *different* classification within a session — for the soft-tail bucket only, and only in the direction of more classifier calls (never fewer), so it can only recover additional in-corpus topics, not lose previously-supported ones. A live eval-canary confirmation is still owed per this repo's RAG-ranking-protection rules before this is fully trusted in production. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc
…ts (#1646) * fix(rag): stop caching soft-tail unsupported-short-circuit zero results The soft-tail bucket of the unsupported short-circuit (low confidence, few expanded terms, no deterministic exclusion match) can be a false negative for genuinely in-corpus bare topics — corpus grounding only credits a document *title* match as a topic anchor, so a term like "catatonia" that is well represented in chunk content but never appears in a document title returns "inconclusive" rather than "in_corpus_topic", and falls through to a nondeterministic LLM classifier call. Caching that zero made a single unlucky classification sticky for every later caller within the cache TTL. searchChunksWithTelemetry now skips the cache write only for that specific soft-tail bucket (isUnsupportedSoftTailAnalysis); the three deterministic exclusion patterns ahead of it in shouldShortCircuitUnsupportedSearch are stable true negatives and stay cached as before. Also exposes telemetry.corpus_grounding on the /api/search response, which was already computed internally (rag.ts) but dropped by the route's hand-picked telemetry subset — needed to diagnose this class of false negative without direct Supabase access. No retrieval/ranking decision logic changed — this is a caching-write decision plus an additive observability field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * docs(ledger): record review for PR #1646 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * fix(rag): forward corpus_grounding telemetry on shared search-cache hits getSharedCachedSearch reconstructs its returned telemetry from a hand-picked field allowlist that dropped corpus_grounding, so the diagnostic field added in the previous commit went missing on cross-process shared-cache hits even though it was stored in the cached payload. Forward it like every other optional telemetry field on that path. Addresses a P2 finding from automated PR review on #1646. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * fix(rag): do not memoize a rejected soft-tail classifier verdict analyzeQueryWithClassifierFallback memoizes every classifier verdict — accepted or rejected — for 15 minutes so the same query gets a consistent result within a session (Finding #11 interim fix). For the soft-tail bucket (the same fragile, low-confidence case the unsupported short-circuit treats specially), that made a rejected verdict sticky for 15 minutes, 15x longer than the 60s search-cache TTL the previous commit stopped writing to — so that fix alone did not meaningfully unstick a repeat "catatonia"-style query. A rejected verdict for that specific bucket is no longer memoized, so a repeat query gets a fresh classifier attempt instead of reproducing the same rejection for the rest of the TTL window. Accepted verdicts, and rejected verdicts outside the soft-tail bucket, keep the existing determinism guarantee unchanged. Addresses a P1 finding from automated PR review on #1646. RAG impact: this changes how often the *same* query can get a *different* classification within a session — for the soft-tail bucket only, and only in the direction of more classifier calls (never fewer), so it can only recover additional in-corpus topics, not lose previously-supported ones. A live eval-canary confirmation is still owed per this repo's RAG-ranking-protection rules before this is fully trusted in production. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * fix(rag): bound the classifier-memo skip and stop skipping deterministic out_of_corpus caching Two follow-up findings from Devin review on the previous commit: - The rejected-soft-tail-verdict memo skip was unbounded: every repeat of a never-recovering query re-invoked the paid OpenAI classifier forever, and a borderline query could flip between zero and non-zero results on every single request instead of within a stable window. Rejected soft-tail verdicts now use a short 60s memo TTL (matching the default search-cache TTL) instead of skipping the memo entirely, bounding both the retry rate and the classifier-call cost while still recovering much faster than the original 15-minute TTL. - isUnsupportedSoftTailAnalysis only inspects the query text and deterministic analysis, not queryAnalysis.corpusGrounding, so a query with a genuine "out_of_corpus" verdict (a deterministic, corpus-derived true negative reached without any LLM call) was also excluded from the search-cache write, forcing every repeat to redo the corpus-grounding and trigram-correction RPCs for an answer that will never change. Excluded "out_of_corpus" from the cache-write skip so it caches like the other deterministic exclusion patterns. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * fix(rag): only skip the soft-tail cache write when a classifier was reachable analyzeQueryWithClassifierFallback returns before any classifier call when OPENAI_API_KEY is absent (rag.ts:1139), so in source-only/offline deployments the soft-tail short-circuit outcome is fully deterministic: same query, same corpus-grounding verdict, same empty result every time. The cache-write skip from the previous two commits didn't account for this, so every repeat of the same query in that deployment mode redid classifyCorpusGrounding (and, when not source-only, the trigram-correction RPC) for an answer that could never change. The skip is now additionally gated on OPENAI_API_KEY being present, since that's the only condition under which a nondeterministic classifier call could actually have produced the verdict. Updated the existing test that had been (correctly, at the time) pinning the no-key case to the skip behavior, and added a new test proving the with-key case — the genuinely nondeterministic one — still skips the cache write. Addresses a further Devin review finding on PR #1646. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ni5rjjPcnbYUg8m8xaFhgc * fix(rag): skip soft-tail unsupported answer cache and harden soft-tail tests The search-layer soft-tail skip left /api/answer still caching the same unsupported refusal for RAG_ANSWER_CACHE_TTL_MS (5 minutes). Extract the shared skip predicate into rag-query-guard and apply it on the unsupported answer path when the empty result came from the soft-tail short circuit and a classifier was reachable. Also pin soft-tail vs non-soft-tail fixtures with isUnsupportedSoftTailAnalysis, drop the duplicate non-soft-tail memo test, and stop asserting setCachedSearch on the in-corpus rescue path (it could pass for the wrong reason). Ratchets the rag.ts maintainability budget to 4362 for the answer-path call site; the decision logic lives in rag-query-guard.ts. Co-Authored-By: Cursor Grok 4.5 <noreply@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs(ledger): record soft-tail answer-cache fix review for PR #1646 Co-Authored-By: Cursor Grok 4.5 <noreply@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
Verification
Notes