fix: codebase-review remediations (clinical-path numeric-verify, coalescing, security hardening)#510
Conversation
…security) Findings from a full-codebase review (3 audits), verified line-by-line and fixed with regression tests: - P1 numeric-verification corpus parity: the model answer path verified numbers against the packed generation context but re-verified at finalize against the unpacked answer.sources, silently blanking correct dose/threshold answers whose figure lived only in a neighbour chunk's adjacent_context. New attachAdjacentContext() rebuilds the packed corpus, threaded as an optional verificationSources param through finalizeRagAnswerQuality -> applyNumericVerification (answer.sources stays unpacked for the client/eval boundary). - P2 answer-coalescing isolation: a coalesced caller no longer inherits the originating request's abort/failure (was a 500 or shared fallback); it catches and falls through to an independent uncoalesced run. - setup-status hardening: production (non-local) origins get coarse per-check detail unless they present the operator deep-probe token; extracted shared deep-probe-auth.ts reused by /api/health. - CSP img-src/media-src narrowed from https: to https://*.supabase.co; document_upload rate limit now fails closed on limiter unavailability (was N x per-instance). - Tooling: lint --max-warnings 0 (blocks exhaustive-deps regressions); removed stale @types/pdf-parse (pdf-parse v2 self-types) with lockfile synced. Offline gates green: eslint --max-warnings 0, tsc --noEmit, 98 affected-file vitest tests. Provider-backed evals (eval:retrieval:quality, eval:rag) and verify:ui not run here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe changes centralize deep-probe authorization, redact setup diagnostics, tighten rate-limit fallback rules, preserve packed RAG context during verification, isolate failed coalesced requests, restrict CSP media origins, and update lint and ignore configuration. ChangesSecurity hardening
RAG verification and request isolation
CSP and repository tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AnswerRequest
participant attachAdjacentContext
participant finalizeRagAnswerQuality
participant applyNumericVerification
AnswerRequest->>attachAdjacentContext: Build verification sources from packed context
attachAdjacentContext-->>AnswerRequest: Return sources with adjacent_context
AnswerRequest->>finalizeRagAnswerQuality: Finalize answer with verification sources
finalizeRagAnswerQuality->>applyNumericVerification: Verify numeric tokens
applyNumericVerification-->>AnswerRequest: Return verified RAG answer
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: e3caf935f5
ℹ️ 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".
![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)
tests/api-rate-limit-fallback.test.ts (1)
108-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLGTM overall — correctly asserts the fail-closed vs fallback-allowed contract for owner subjects.
One coverage gap worth closing: neither test exercises the
isLocalNoAuthMode() === truebranch fordocument_upload, which per the source comment ("Local-no-auth dev keeps the in-memory fallback for single-instance usability") should still allow fallback. Adding that case would pin down the full fail-closed/fallback matrix for this security-sensitive bucket.✅ Suggested additional test
+ it("still allows in-memory fallback for document_upload in local-no-auth mode", async () => { + vi.stubEnv("NODE_ENV", "development"); + vi.doMock("`@/lib/env`", () => ({ + isLocalNoAuthMode: () => true, + })); + const { consumeSubjectApiRateLimit } = await import("../src/lib/api-rate-limit"); + const supabase = { + rpc: vi.fn(async () => ({ data: null, error: { code: "PGRST202", message: "missing RPC" } })), + }; + + const result = await consumeSubjectApiRateLimit({ + supabase: supabase as never, + subject: { kind: "owner", ownerId: "owner-1" }, + bucket: "document_upload", + allowInMemoryFallbackOnUnavailable: true, + }); + + expect(result.limited).toBe(false); + });🤖 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/api-rate-limit-fallback.test.ts` around lines 108 - 151, Add a test in the “document_upload fail-closed limiter” suite covering isLocalNoAuthMode() === true, with the durable limiter unavailable and in-memory fallback enabled. Assert that consumeSubjectApiRateLimit resolves with an un limited result, while preserving the existing production fail-closed and document_read fallback cases.src/lib/rag.ts (2)
3603-4922: 🧹 Nitpick | 🔵 TrivialClinical governance preflight reminder.
This change touches answer generation and clinical numeric-dose verification (adjacent-context packing, numeric faithfulness gate, coalesced-answer fallback). As per path instructions, PRs touching answer generation or clinical output must complete the clinical governance preflight and relevant production-readiness checks before merge — the PR objectives note provider-backed RAG evaluations still need to run.
🤖 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 3603 - 4922, Complete the required clinical governance preflight and production-readiness checks for the answer generation and numeric verification changes in answerQuestionWithScope and answerQuestionWithScopeUncoalesced. Run the provider-backed RAG evaluations and document or resolve any findings before merging; do not alter the implementation solely for this reminder.Source: Path instructions
3615-3630: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the try/catch to only the coalesced-fetch, not the post-processing.
The catch is meant to isolate this caller from the originating request's failure, but it also swallows any error thrown by the subsequent
routingReason/latencyTimingsmutation. A bug there would silently look like "the in-flight request failed" and trigger a full duplicate (and possibly expensive) uncoalesced OpenAI run instead of surfacing the real error.♻️ Suggested narrowing
- try { - const answer = cloneAnswer(await existing); - answer.routingReason = answer.routingReason - ? `${answer.routingReason}; answer_inflight_coalesced` - : "answer_inflight_coalesced"; - answer.latencyTimings = { - ...answer.latencyTimings, - total_latency_ms: Date.now() - startedAt, - }; - return answer; - } catch { + let coalescedAnswer: RagAnswer | null = null; + try { + coalescedAnswer = cloneAnswer(await existing); + } catch { // The in-flight request we coalesced onto failed — most often because the ORIGINATING // caller aborted mid-flight (its AbortSignal is not ours) or its search phase threw. Do // not propagate another caller's failure to this still-connected request: fall through to // an independent, uncoalesced run so this request is decided only by its own inputs. } + if (coalescedAnswer) { + coalescedAnswer.routingReason = coalescedAnswer.routingReason + ? `${coalescedAnswer.routingReason}; answer_inflight_coalesced` + : "answer_inflight_coalesced"; + coalescedAnswer.latencyTimings = { + ...coalescedAnswer.latencyTimings, + total_latency_ms: Date.now() - startedAt, + }; + return coalescedAnswer; + }🤖 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 3615 - 3630, In the coalesced-response path, narrow the try/catch to only the await of existing used to obtain the cloned answer; perform the routingReason and latencyTimings updates and return outside the catch so post-processing errors propagate instead of triggering an independent run.
🤖 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 `@src/lib/deep-probe-auth.ts`:
- Around line 10-19: Update allowDeepHealthProbe to compare the byte lengths of
the UTF-8 expected and received buffers before calling timingSafeEqual, rather
than comparing token.length with secret.length. Return false when the buffer
lengths differ so malformed or non-ASCII tokens are denied without throwing.
---
Nitpick comments:
In `@src/lib/rag.ts`:
- Around line 3603-4922: Complete the required clinical governance preflight and
production-readiness checks for the answer generation and numeric verification
changes in answerQuestionWithScope and answerQuestionWithScopeUncoalesced. Run
the provider-backed RAG evaluations and document or resolve any findings before
merging; do not alter the implementation solely for this reminder.
- Around line 3615-3630: In the coalesced-response path, narrow the try/catch to
only the await of existing used to obtain the cloned answer; perform the
routingReason and latencyTimings updates and return outside the catch so
post-processing errors propagate instead of triggering an independent run.
In `@tests/api-rate-limit-fallback.test.ts`:
- Around line 108-151: Add a test in the “document_upload fail-closed limiter”
suite covering isLocalNoAuthMode() === true, with the durable limiter
unavailable and in-memory fallback enabled. Assert that
consumeSubjectApiRateLimit resolves with an un limited result, while preserving
the existing production fail-closed and document_read fallback cases.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95eda418-1ae6-41e7-87a8-0f6c64a40522
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
eslint.config.mjspackage.jsonsrc/app/api/health/route.tssrc/app/api/setup-status/route.tssrc/lib/answer-verification.tssrc/lib/api-rate-limit.tssrc/lib/deep-probe-auth.tssrc/lib/rag-cache.tssrc/lib/rag-extractive-answer.tssrc/lib/rag.tssrc/lib/security-headers.tstests/api-rate-limit-fallback.test.tstests/proxy.test.tstests/rag-answer-fallback.test.tstests/rag-content-accuracy.test.tstests/security-headers.test.tstests/setup-status-route.test.tstests/ui-smoke.spec.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @BigSimmo. * #510 (comment) The following files were modified: * `src/app/api/health/route.ts` * `src/app/api/setup-status/route.ts` * `src/lib/answer-verification.ts` * `src/lib/api-rate-limit.ts` * `src/lib/deep-probe-auth.ts` * `src/lib/rag-cache.ts` * `src/lib/rag-extractive-answer.ts` * `src/lib/rag.ts` * `src/lib/security-headers.ts`
- setup-status: gate raw per-check detail on a TRUSTED server runtime signal (NODE_ENV + demo/local-no-auth mode) instead of request.url's host. request.url reflects the client-supplied Host behind a proxy, so a spoofed localhost:<managed-port> could otherwise unlock raw Supabase/project detail without the operator token (Codex P2). - deep-probe-auth: compare UTF-8 byte lengths before timingSafeEqual. A crafted multi-byte x-health-deep-token with a matching code-unit length but a different byte length made timingSafeEqual throw (RangeError -> crafted-header 500) (CodeRabbit, Major). - Tests: a production request with a local-looking managed-port Host stays coarse; a crafted multi-byte token is rejected without throwing. Live validation: golden retrieval 36/36; eval:rag 14/15 grounded (the one miss is a pre-existing discharge doc-expectation mismatch, numericWarnings=0) with no numeric-faithfulness-gate suppressions. typecheck + lint (--max-warnings 0) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a26e238b4
ℹ️ 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".
| } catch { | ||
| // The in-flight request we coalesced onto failed — most often because the ORIGINATING | ||
| // caller aborted mid-flight (its AbortSignal is not ours) or its search phase threw. Do | ||
| // not propagate another caller's failure to this still-connected request: fall through to | ||
| // an independent, uncoalesced run so this request is decided only by its own inputs. | ||
| } |
There was a problem hiding this comment.
When two or more identical callers are awaiting the same existing promise and the originating request aborts, every waiter enters this catch and then falls through to create its own pending request without rechecking answerInflight. That turns one aborted in-flight answer into N independent retrieval/OpenAI generations instead of one shared replacement for the still-connected callers, multiplying paid/provider and DB load under concurrent retries; a regression with two coalesced waiters after one aborted origin should assert only one replacement generation.
Useful? React with 👍 / 👎.
Docstrings generation was requested by @BigSimmo. * #510 (comment) The following files were modified: * `src/app/api/health/route.ts` * `src/app/api/setup-status/route.ts` * `src/lib/answer-verification.ts` * `src/lib/api-rate-limit.ts` * `src/lib/deep-probe-auth.ts` * `src/lib/rag-cache.ts` * `src/lib/rag-extractive-answer.ts` * `src/lib/rag.ts` * `src/lib/security-headers.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Remediations from a full-codebase review (three parallel audits: pipeline correctness, security/data-access, frontend/config). Each finding below was verified line-by-line and ships with a regression test.
adjacent_context), then swappedanswer.sourcesto the unpackedanswerInputResultsbeforefinalizeRagAnswerQualityre-verified — silently blanking correct dose/threshold answers whose figure lived only in a neighbour chunk'sadjacent_context. NewattachAdjacentContext()(rag-cache.ts) rebuilds the packed corpus, threaded as an optionalverificationSourcesparam throughfinalizeRagAnswerQuality→applyNumericVerification.answer.sourcesstays unpacked, preserving the client/eval byte boundary. The fail-closed behaviour for genuinely unverifiable numbers is unchanged.answerQuestionWithScopecoalesced identical concurrent requests onto one promise with no error isolation, so an originating caller's abort or search-phase throw returned a 500 (or a shared fallback) to unrelated concurrent callers. The coalesced awaiter now catches and falls through to an independent uncoalesced run./api/setup-statushardening. Non-local (production) origins receive coarse per-checkdetail(raw Supabase error text and project posture redacted) unless they present the operatorx-health-deep-token. Extracted a sharedsrc/lib/deep-probe-auth.tsreused by/api/health. The client's onlydetailreader isNODE_ENV !== "production"-gated, so the UI is unaffected.img-src/media-srcnarrowed from anyhttps:tohttps://*.supabase.co(the app loads no other cross-origin media).document_uploadnow fails closed (503) on limiter unavailability likeanswer, instead of falling back to a per-instance limiter that grants N× the limit across horizontally-scaled instances during a limiter outage.lint --max-warnings 0(makesreact-hooks/exhaustive-depsblocking; repo currently has 0 warnings). Removed stale@types/pdf-parse(pdf-parse v2 ships its own types);package-lock.jsonsynced sonpm cistays valid. Deduped eslintglobalIgnores.Full findings report (including three items deferred with recipes) is in the plan file.
Verification
Ran here (this worktree has no local
node_modules; run against the repo binaries), all green:eslint … --max-warnings 0→ cleantsc --noEmit→ cleanvitest runover all 9 affected/added test files → 98 tests pass (new tests: numeric-verification corpus parity +attachAdjacentContext; coalescing abort isolation;setup-statuscoarse-detail gating;document_uploadfail-closed limiter; tightened CSPimg-srcassertions).Not run here — please run before merge (environment/provider constraints):
npm run verify:pr-local(full gate — needs a complete install/build env)npm run verify:ui— CSPimg-srcnarrowing is browser-visible; confirm Supabase Storage images still rendernpm run eval:retrieval:quality(must stay 36/36) — retrieval-adjacent (attachAdjacentContextfeeds the verification corpus); provider-backed, cannot run herenpm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only— the numeric-faithfulness gate (answer post-processing) changed; grounded-supported should rise or hold (this fixes false suppressions) and citation-failure must stay 0npm run check:production-readiness— CSP, rate-limit, and setup-status touch production env/privacyClinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy) — unchangedsetup-statuschange reduces exposure;deep-probe-auth.tsis server-only)Notes
next build),@axe-core/playwrighta11y + coverage ratchet (needs a dependency install + Chromium/coverage runs), and large-file decomposition +tsconfig noUncheckedIndexedAccess(large, incremental).docs/database-drift-detection.md(live bodies are ahead of the repo; the fix needs a live-body generation script + operator apply, and the naive rewrite was neutralized as regressive). The app-layer owner-scope floor is already safe.🤖 Generated with Claude Code