From ff7961826fe38f18dc552bdc2d0cdbda43ad0810 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:34:32 -0700 Subject: [PATCH] feat(rag): codify the vector store as a migration + assert it exists (#481, part of #482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `course_chunks`, the `match_course_chunks` RPC and `CREATE EXTENSION vector` appeared in ZERO .sql files in this repo, yet all three are live in staging and production. Any database replayed purely from `python -m db.migrate` — local Supabase, the E2E stack, a fresh environment — therefore had RAG dead end to end, silently: retrieve_chunks swallows the RPC failure into [], _get_catalog_chunk degrades to "", indexing failures vanish into a fire-and-forget log line. The tutor just answers ungrounded, with no error, no metric and no user-visible signal. Migration 0039 codifies the extension, the table, its indexes and the RPC. Shape verified against LIVE production and staging by reading a real row and calling the RPC — columns, the 768-dim embedding, and the RPC's exact parameter and return names — rather than from the design doc, which describes intent while the database is what the code actually talks to. (The code's _OUTPUT_DIM is 768 and matches; a 3072-dim probe is rejected by the live RPC.) Every statement is IF NOT EXISTS / CREATE OR REPLACE, mirroring 0032's reconcile pattern, so it is a no-op where the objects already exist. Adds a `ragstore` oracle asserting the store EXISTS — the gap the issue names ("nothing in the suites or oracles asserts the table exists"). It checks the extension, the table and the RPC, deliberately not their contents: an empty course_chunks is normal on a fresh stack, a missing one is the bug. It also counts rows with a NULL embedding, which led to the write-path half. index_document_chunks upserted records whose embedding never landed — match_course_chunks ranks by vector distance and skips NULLs, so those rows were unretrievable by construction while still counting toward the "indexed N chunks" the caller logs. That is how a total embedding outage read as a complete success. It now drops them, logs how many, and reports only what was really indexed (part of #482). Two tests changed because they pinned that bug rather than a contract: test_index_document_chunks_handles_embedding_failure asserted the NULL rows were upserted, and the function-mode egress test asserted the same shape incidentally — its real subject, the absence of transport egress, is unchanged. part of #481 Co-Authored-By: Claude Fable 5 --- .../db/migrations/0039_rag_vector_store.sql | 86 +++++++++++++++++++ backend/e2e_oracles/__main__.py | 3 +- backend/e2e_oracles/gather.py | 72 ++++++++++++++++ backend/services/rag_service.py | 20 ++++- backend/tests/test_rag_service.py | 62 +++++++++---- 5 files changed, 224 insertions(+), 19 deletions(-) create mode 100644 backend/db/migrations/0039_rag_vector_store.sql diff --git a/backend/db/migrations/0039_rag_vector_store.sql b/backend/db/migrations/0039_rag_vector_store.sql new file mode 100644 index 00000000..0d3d8306 --- /dev/null +++ b/backend/db/migrations/0039_rag_vector_store.sql @@ -0,0 +1,86 @@ +-- 0039: codify the RAG vector store that only ever existed as dashboard DDL (#481). +-- +-- `course_chunks`, the `match_course_chunks` RPC and `CREATE EXTENSION vector` +-- appear in ZERO .sql files in this repo, yet all three are live in staging and +-- production. A database replayed purely from `python -m db.migrate` — local +-- Supabase, the E2E stack, any fresh environment — therefore has RAG silently +-- dead end to end: +-- +-- * retrieve_chunks() swallows the RPC failure into [] (rag_service.py) +-- * _get_catalog_chunk() degrades to "" +-- * indexing failures vanish into a fire-and-forget log line +-- +-- so the tutor answers ungrounded with no error, no metric and no user-visible +-- signal, and nothing in the suites or oracles asserts the table exists. +-- +-- Shape verified against LIVE production and staging on 2026-07-31 by reading a +-- real row and calling the RPC (columns, the 768-dim embedding, and the RPC's +-- exact parameter and return names), rather than from the design doc alone — +-- the doc describes intent, the database is what the code actually talks to. +-- +-- IF NOT EXISTS throughout, mirroring 0032's reconcile pattern: this must be a +-- no-op on staging and production, where every object below already exists. + +CREATE EXTENSION IF NOT EXISTS vector; + +-- Chunk ids are content-addressed (sha256 hex) so a re-upload of the same text +-- merges instead of duplicating — see rag_service.chunk_id. +CREATE TABLE IF NOT EXISTS course_chunks ( + id TEXT PRIMARY KEY, + course_id TEXT NOT NULL, + doc_id TEXT, + uploader_id TEXT, + chunk_index INTEGER, + chunk_text TEXT, + chunk_hash TEXT, + embedding VECTOR(768), + category TEXT, + semester TEXT, + section_id TEXT, + school TEXT, + created_at TIMESTAMPTZ DEFAULT now() +); + +-- Retrieval always filters by course before ranking. +CREATE INDEX IF NOT EXISTS idx_course_chunks_course_id ON course_chunks (course_id); +CREATE INDEX IF NOT EXISTS idx_course_chunks_doc_id ON course_chunks (doc_id); + +-- ANN index. ivfflat needs training data to be worth building, and `lists` +-- wants tuning against real row counts; cosine matches the normalised +-- gemini-embedding-001 output the code stores. +CREATE INDEX IF NOT EXISTS idx_course_chunks_embedding + ON course_chunks USING ivfflat (embedding vector_cosine_ops) WITH (lists = 100); + +-- Retrieval RPC. Parameter names are part of the contract — rag_service.py +-- calls this by keyword (query_embedding / match_count / filter_course_id) and +-- reads back id, course_id, chunk_text, category, similarity. +-- +-- CREATE OR REPLACE rather than IF NOT EXISTS: functions have no IF NOT EXISTS +-- form, and replacing with the identical body is the no-op we want on the +-- environments that already have it. +CREATE OR REPLACE FUNCTION match_course_chunks( + query_embedding VECTOR(768), + match_count INTEGER DEFAULT 5, + filter_course_id TEXT DEFAULT NULL +) +RETURNS TABLE ( + id TEXT, + course_id TEXT, + chunk_text TEXT, + category TEXT, + similarity FLOAT +) +LANGUAGE sql STABLE +AS $$ + SELECT + c.id, + c.course_id, + c.chunk_text, + c.category, + 1 - (c.embedding <=> query_embedding) AS similarity + FROM course_chunks c + WHERE c.embedding IS NOT NULL + AND (filter_course_id IS NULL OR c.course_id = filter_course_id) + ORDER BY c.embedding <=> query_embedding + LIMIT match_count; +$$; diff --git a/backend/e2e_oracles/__main__.py b/backend/e2e_oracles/__main__.py index 0144e7f9..a191e320 100644 --- a/backend/e2e_oracles/__main__.py +++ b/backend/e2e_oracles/__main__.py @@ -5,7 +5,7 @@ venv/bin/python -m e2e_oracles [--json] [--check NAME]... [--user ID] \\ [--base-url URL] [--log PATH] -Check names: `graph`, `counts`, `ciphertext`, `logscan`, `orphans` — default +Check names: `graph`, `counts`, `ciphertext`, `logscan`, `orphans`, `ragstore` — default is all five, in that (sorted) order. `--check` may repeat to select a subset. Exit codes: 0 clean / 1 findings / 2 infra error — ANY `Finding(oracle= @@ -44,6 +44,7 @@ "ciphertext": lambda args: gather.run_ciphertext(args), "logscan": lambda args: gather.run_logscan(args), "orphans": lambda args: gather.run_orphans(args), + "ragstore": lambda args: gather.run_ragstore(args), } diff --git a/backend/e2e_oracles/gather.py b/backend/e2e_oracles/gather.py index ef2e4b4e..a06b5029 100644 --- a/backend/e2e_oracles/gather.py +++ b/backend/e2e_oracles/gather.py @@ -283,3 +283,75 @@ def run_logscan(args: argparse.Namespace) -> tuple[list[Finding], int]: ) ], 0 return scan_file(log_path) + + +def run_ragstore(args: argparse.Namespace) -> tuple[list[Finding], int]: + """`ragstore`: the RAG vector store exists and is queryable (#481). + + The failure this exists to catch is SILENT. `course_chunks`, the + `match_course_chunks` RPC and the `vector` extension lived only as + dashboard DDL, in no migration — so a database replayed purely from + `python -m db.migrate` had RAG dead end to end while every suite stayed + green: `retrieve_chunks` swallows the RPC failure into `[]`, + `_get_catalog_chunk` degrades to `""`, and indexing failures vanish into + a fire-and-forget log line. The tutor just answers ungrounded. + + So this asserts the store's EXISTENCE, not its contents — an empty + course_chunks is normal on a fresh stack; a missing one is the bug. + """ + conn = _db_conn() + findings: list[Finding] = [] + + ext = conn.execute("SELECT 1 FROM pg_extension WHERE extname = 'vector'").fetchone() + if not ext: + findings.append( + Finding( + oracle="ragstore", + summary="pgvector extension is not installed — RAG cannot store or query embeddings", + evidence={"expected": "CREATE EXTENSION vector (migration 0039)"}, + ) + ) + + tbl = conn.execute( + "SELECT 1 FROM information_schema.tables " + "WHERE table_schema = 'public' AND table_name = 'course_chunks'" + ).fetchone() + if not tbl: + findings.append( + Finding( + oracle="ragstore", + summary="course_chunks table is missing — retrieval degrades to [] with no error", + evidence={"expected": "migration 0039 creates it"}, + ) + ) + + fn = conn.execute( + "SELECT 1 FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace " + "WHERE p.proname = 'match_course_chunks'" + ).fetchone() + if not fn: + findings.append( + Finding( + oracle="ragstore", + summary="match_course_chunks RPC is missing — every retrieval silently returns []", + evidence={"expected": "migration 0039 creates it"}, + ) + ) + + # Rows whose embedding never landed are retrievable by nothing: the RPC + # orders by distance and skips NULLs, so they are dead weight that still + # counts as "indexed" to the caller (#482). + if tbl: + dead = conn.execute( + "SELECT count(*) AS n FROM course_chunks WHERE embedding IS NULL" + ).fetchone() + if dead and dead["n"]: + findings.append( + Finding( + oracle="ragstore", + summary=f"{dead['n']} course_chunks row(s) have a NULL embedding — indexed but unretrievable", + evidence={"null_embedding_rows": dead["n"]}, + ) + ) + + return findings, 0 diff --git a/backend/services/rag_service.py b/backend/services/rag_service.py index 8294084a..3bb5e4c6 100644 --- a/backend/services/rag_service.py +++ b/backend/services/rag_service.py @@ -210,8 +210,24 @@ def index_document_chunks( except Exception as e: print(f"[RAG] embed failed for doc {doc_id} batch {i}: {e}") - table("course_chunks").upsert(records, on_conflict="id") - return len(records) + # Never persist a chunk whose embedding didn't land. match_course_chunks + # ranks by vector distance, so a NULL-embedding row can never be returned + # by retrieval — it is dead weight that nonetheless counted toward the + # "indexed N chunks" the caller logs, which is how a partial embedding + # failure used to read as a complete success (#482). + embedded = [r for r in records if r["embedding"] is not None] + dropped = len(records) - len(embedded) + if dropped: + print( + f"[RAG] doc {doc_id}: dropped {dropped}/{len(records)} chunk(s) " + f"with no embedding — they would be unretrievable" + ) + + if not embedded: + return 0 + + table("course_chunks").upsert(embedded, on_conflict="id") + return len(embedded) def format_rag_context(chunks: list[dict]) -> str: diff --git a/backend/tests/test_rag_service.py b/backend/tests/test_rag_service.py index 38a3ae92..c5b95a7c 100644 --- a/backend/tests/test_rag_service.py +++ b/backend/tests/test_rag_service.py @@ -191,8 +191,17 @@ def test_duplicate_chunks_within_one_document_are_deduped(mock_client): @patch("services.rag_service._embed_documents_batch") -def test_index_document_chunks_handles_embedding_failure(mock_embed): - """Test that embedding failures are caught and records are still upserted with embedding=None.""" +def test_index_document_chunks_drops_chunks_whose_embedding_failed(mock_embed): + """A total embedding failure must persist NOTHING and report 0 (#482). + + This test previously asserted the opposite — that the rows were upserted + with `embedding=None` and the call reported the full chunk count. That was + pinning a bug, not a contract: `match_course_chunks` ranks by vector + distance, so a NULL-embedding row can never be returned by retrieval. Those + rows were unreachable dead weight that still counted toward the "indexed N + chunks" the caller logs, which is exactly how a total embedding outage read + as a complete success. + """ mock_embed.side_effect = Exception("API error") with patch("services.rag_service.table") as mock_table: mock_table.return_value.upsert.return_value = [] @@ -205,16 +214,33 @@ def test_index_document_chunks_handles_embedding_failure(mock_embed): chunks=["chunk one", "chunk two", "chunk three"], ) - # Function completes without raising an exception - assert count == 3 + # Still no exception — indexing stays best-effort, as before. + assert count == 0 + # …but nothing unretrievable is written. + mock_table.return_value.upsert.assert_not_called() - # Records were still upserted - mock_table.return_value.upsert.assert_called_once() - upsert_records = mock_table.return_value.upsert.call_args[0][0] - # All records have embedding=None since embedding failed - assert all(rec["embedding"] is None for rec in upsert_records) - assert len(upsert_records) == 3 +@patch("services.rag_service._embed_documents_batch") +def test_index_document_chunks_keeps_the_chunks_that_did_embed(mock_embed): + """A PARTIAL failure keeps the good chunks and drops only the bad ones.""" + # Batch size is 50, so five chunks arrive as one batch; return a short + # vector list so zip() leaves the tail without an embedding. + mock_embed.return_value = [[0.1] * 768, [0.2] * 768] + with patch("services.rag_service.table") as mock_table: + mock_table.return_value.upsert.return_value = [] + from services.rag_service import index_document_chunks + + count = index_document_chunks( + course_code="CAS CS 330", + doc_id="doc-partial", + uploader_id="user-xyz", + chunks=["one", "two", "three", "four", "five"], + ) + + assert count == 2 + upserted = mock_table.return_value.upsert.call_args[0][0] + assert len(upserted) == 2 + assert all(rec["embedding"] is not None for rec in upserted) # ── #439: below-seam RAG embed calls must gate on SAPLING_MODEL_MODE ─────── @@ -290,9 +316,14 @@ def test_retrieve_chunks_never_reaches_transport_in_function_mode(monkeypatch, c def test_index_document_chunks_never_reaches_transport_in_function_mode(monkeypatch, capsys): """Same proof as above for the batch/document embed path used by document - upload indexing — the deterministic no-op is count-with-embedding=None, - same shape as test_index_document_chunks_handles_embedding_failure, but - now reached by the mode gate rather than a real API error.""" + upload indexing: the mode gate stops it before google-genai's transport. + + The deterministic no-op is now "write nothing, report 0". It used to be + "write rows with embedding=None and report the full count" — but in + function mode no embedding can be produced, so those rows were dead on + arrival: unretrievable by construction, and flagged by the `ragstore` + oracle. What this test guards is the absence of egress, which is unchanged. + """ monkeypatch.setenv("SAPLING_MODEL_MODE", "function") from services.rag_service import index_document_chunks @@ -305,9 +336,8 @@ def test_index_document_chunks_never_reaches_transport_in_function_mode(monkeypa chunks=["chunk one", "chunk two"], ) - assert count == 2 - upsert_records = mock_table.return_value.upsert.call_args[0][0] - assert all(rec["embedding"] is None for rec in upsert_records) + assert count == 0 + mock_table.return_value.upsert.assert_not_called() captured = capsys.readouterr() assert "unstubbed LLM egress" not in captured.out assert "SAPLING_MODEL_MODE" in captured.out