Skip to content

Fix security, resilience, and deduplication issues across AI chat and sync pipeline#4

Open
yassinekolsi with Copilot wants to merge 2 commits into
yousseffrom
copilot/sub-pr-3
Open

Fix security, resilience, and deduplication issues across AI chat and sync pipeline#4
yassinekolsi with Copilot wants to merge 2 commits into
yousseffrom
copilot/sub-pr-3

Conversation

Copilot AI commented Mar 19, 2026

Copy link
Copy Markdown

Eight issues flagged in code review: a service-role authorization bypass, a PostgREST empty-in() crash, SQLite/JSONB type mismatch, un-retried network errors, a silent storage-path parse failure, a missing RLS cross-clinic check, @ts-nocheck masking type errors, and duplicated context-selection logic.

Authorization & RLS

  • ai-chat-patient: Removed doctorProfile fallback that let any authenticated user hit patient records via service role. Now returns 403 immediately if no doctor row exists for user.id.
  • 004_ai_chat.sql: Added EXISTS (SELECT 1 FROM patients p WHERE p.id = patient_id AND p.clinic_id = clinic_id) to the ai_conversations INSERT policy — previously a doctor could reference a patient from a different clinic.

Query correctness

  • ai-chat-patient: Replaced nested inline sessions subquery in .in() with an explicit two-step fetch. Empty session arrays no longer reach PostgREST and trigger a 400 parse error; sessions and records errors are surfaced independently.
// Before — crashes when patient has no sessions
.in('session_id', (await supabase.from('sessions')).data?.map() ?? [])

// After — skips records query entirely on empty session list
const { data: sessions, error: sessionsErr } = await supabaseService.from('sessions')
if (sessionsErr) return jsonResponse({ error: '…' }, 500);
const sessionIds = (sessions ?? []).map(s => s.id);
if (sessionIds.length > 0) { /* records query */ }

Data integrity

  • cloudSync.ts: cloudRecord.extracted_data arrives from Postgres as a JSONB object; updateRecordExtraction expects a JSON string for SQLite. Now normalizes with JSON.stringify when the value is not already a string.

Resilience

  • geminiService.ts: fetch() was not wrapped in try/catch, so network errors (DNS, TLS) bypassed the retry loop entirely. Now caught and retried with the same exponential backoff. Extracted BASE_RETRY_DELAY_MS = 1500 constant.
  • extract-handwriting: toStoragePath returned the original URL verbatim when the storage marker was absent, silently passing an invalid path to storage.download(). Now returns '', which the existing guard converts to a clear 400.

Type safety & deduplication

  • ai-chat-patient: Removed // @ts-nocheck; added top-level RecordRow type.
  • aiChat.ts: Replaced duplicated tokenization / scoring / truncation loop with selectTopK from the existing lib/contextSelector.ts (which has Jest coverage).

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@yassinekolsi
yassinekolsi marked this pull request as ready for review March 19, 2026 14:54
Copilot AI review requested due to automatic review settings March 19, 2026 14:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Co-authored-by: yassinekolsi <196232305+yassinekolsi@users.noreply.github.com>
Copilot AI changed the title [WIP] Restore demo-ready chatbot flow and sync resilience Fix security, resilience, and deduplication issues across AI chat and sync pipeline Mar 19, 2026
Copilot AI requested a review from yassinekolsi March 19, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants