Skip to content

feat(wave1): stop the bleeding — data integrity, security, destructive guards#12

Merged
vikranthreddimasu merged 1 commit into
mainfrom
feat/wave1-stop-the-bleeding
Apr 18, 2026
Merged

feat(wave1): stop the bleeding — data integrity, security, destructive guards#12
vikranthreddimasu merged 1 commit into
mainfrom
feat/wave1-stop-the-bleeding

Conversation

@vikranthreddimasu

Copy link
Copy Markdown
Owner

Context

This is Wave 1 of 5 from the UX/UI/Tech rebuild plan. Full plan and audit context:

The audits found 117 issues. Wave 1 closes the 13 that can silently corrupt data, leak files, compromise security, or ship a blank-screen production build. Nothing cosmetic in this PR — every change is load-bearing.

What changes

Data integrity

  • Message ids replace positional indices. updateMessageAt(index, …) was a silent-corruption bug: mid-stream notebook switch + array mutation could write the assistant delta into the wrong slot. Messages now carry a stable UUID assigned at add time; store exposes updateMessage(id, patch), removeMessage(id), setMessages(msgs).
  • Abort is a real abort now. User clicks stop → stream tears down, partial message is either marked aborted or removed if empty, and we do NOT fall through to the non-streaming endpoint (which used to deliver the full reply anyway). AbortError detection uses controller.signal.aborted + an explicit userAborted flag.
  • Stream is canceled on notebook switch. A useEffect cleanup in useChat aborts the controller when activeNotebookId changes. Previously a mid-stream notebook switch would keep streaming into the old conversation while the UI showed a new notebook.
  • clearMessages is decomposed. The single action was called for 4 different intents — new chat, clear chat, notebook switch, conversation load — each with identical but mostly wrong side effects (e.g., always wiping activeSources). Replaced with newChat(), resetMessagesOnly(), setMessages(). Callers now say what they mean.
  • Conversation load fetches before clearing. A network error no longer nukes the current context. Messages swap in one setMessages() call (no N re-renders that looked like streaming). Historical sources are restored from the last message that actually had them, with document_name populated the same way the stream path does.
  • Ingestion source_count accumulates. complete_ingestion used SET source_count = ? with per-job count → a 6th upload set the counter to 1. Now source_count = source_count + ?. Regression test added.

Security

  • CORS locked down. allow_origin_regex=\".*\" + allow_credentials=True was a localhost-CSRF footgun (any browser tab on the machine could hit /api/* with credentials and exfiltrate notebooks). Only the explicit dev origins + Electron's null origin now, credentials off.
  • Document preview path-traversal hardened. The old path-resolution had 4 fallback strategies; strategy 4 iterated every notebook subdirectory and served the first filename match, so notebook A could serve files from notebook B. Now resolution is scoped to uploads/{notebook_id}/, with Path.relative_to() as the final guard. Response also sets Content-Disposition: attachment.
  • app:openExternal validates scheme. Only https?:// and mailto: pass; a renderer-side XSS through markdown can't pivot to file:// or javascript: handlers.

Reliability / cleanup

  • Cascade delete notebook. DELETE /notebooks/:id previously removed the SQLite row but left the Chroma chunk + summary collections and the uploads directory behind — storage grew without bound. Now drops all three. New VectorStoreManager.delete_notebook_collections() helper.
  • Vite base: './' . Without it, packaged Electron loads absolute /assets/... paths from file:// and renders a blank white screen. Bug that only manifested in production.
  • ConfirmDialog for notebook and conversation deletes. One misclick on an overflow menu used to delete an entire notebook (all documents, embeddings, conversations) with zero guard. Now Escape-to-cancel, focus on Cancel by default, Enter only fires on confirm button.
  • CommandPalette Rules of Hooks. useState(selectedIndex) was below an early return. Worked today; would break under React compiler or StrictMode.

Verification

  • `npm run build` clean (tsc + vite, 269 modules, 850ms)
  • `uv run ruff check .` clean
  • `uv run pytest -q` 30 passed, 1 skipped (includes new `test_complete_ingestion_accumulates_counts_across_jobs`)

What's intentionally NOT in this PR

  • Citation trust layer resurrection (Wave 2)
  • Rename notebook, delete document, real upload progress (Wave 3)
  • Backend crash recovery, API timeouts, OOM guards, WAL mode (Wave 4)
  • Design-polish + a11y items (Wave 5)

Each wave is its own PR on green CI. This one is the foundation.

🤖 Generated with Claude Code

…e guards

First of five rebuild waves from .gstack/qa-reports/PLAN.md. The audits
(ux-audit.md, tech-audit.md) found 117 issues; this wave closes the ones
that can silently corrupt user data, leak files, or ship a blank-screen
production build. Each change is load-bearing.

## Data integrity

- app-store: messages now carry a stable client id; updateMessageAt-by-index
  is replaced with updateMessage-by-id so streaming writes survive notebook
  switches, aborts, and setMessages swaps without landing in the wrong slot
  or silently dropping. Store also grows newChat(), resetMessagesOnly(), and
  setMessages() so the previous 4 callers of clearMessages() can each say
  what they actually meant.
- useChat: user-initiated abort no longer falls through to the non-streaming
  fallback (stop button used to deliver the full reply anyway), and no longer
  leaves a partial assistant message in history to be replayed on the next
  send. Empty aborted turns are removed; partial ones are marked aborted.
  Switching notebooks tears down the in-flight stream.
- useConversations.loadConversation fetches BEFORE clearing, so a network
  error no longer wipes the user's current context. Messages are swapped in
  one setMessages() call and sources are restored from the last message that
  actually had them, with document_name populated the same way the streaming
  path does.
- ChatView retry path replaces clearMessages + N addMessage calls with a
  single setMessages slice — no incremental render flicker.
- notebook_store.complete_ingestion now accumulates source_count / chunk_count
  instead of overwriting with per-job values. Previously a 6th upload would
  set the counter to 1. Test added.

## Security

- Backend CORS drops allow_origin_regex=".*" + allow_credentials=True (a
  localhost-CSRF footgun that let any browser tab on the machine exfiltrate
  notebooks). Only the explicit dev origins + Electron's "null" origin now.
- Document preview path resolution is locked to the notebook's scoped
  uploads directory. The old cross-notebook filename-match fallback let
  notebook A serve files from notebook B. Response now also sets
  Content-Disposition: attachment.
- Electron app:openExternal now validates URL scheme — http(s) and mailto
  only. A renderer-side XSS via markdown can't pivot into file:// or
  javascript: handlers.

## Reliability / cleanup

- DELETE /notebooks/:id now also drops the Chroma chunk + summary
  collections and rm -rfs the uploads directory. Prior behavior left
  both behind, leaking disk indefinitely.
- Vite config sets base: './' so the packaged Electron renderer actually
  loads assets from file:// (previously: blank white screen in prod).
- ConfirmDialog component wired to notebook delete and conversation delete.
  Neither destructive action previously had any guard; a misclick on a
  sidebar overflow menu would permanently destroy the notebook and all its
  documents, embeddings, and conversations with no undo.
- CommandPalette useState for selectedIndex moved above the early return —
  Rules of Hooks compliance, future-proofs against React compiler / StrictMode.

Everything lives behind a green CI: backend ruff + pytest clean, desktop
tsc + vite build clean. Waves 2-5 build on this.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant