Repository navigation
chore(phase-0): integration tier, embedding-status fix, evaluation corpus - #10
Draft
VampiricCyborg wants to merge 14 commits into
Draft
VampiricCyborg wants to merge 14 commits into
VampiricCyborg wants to merge 14 commits into
Conversation
The audit still listed sentence-transformers as the embedding dependency. That stack was removed on purpose -- loading PyTorch exhausted memory on Railway's container and crashed the service on boot -- and replaced with onnxruntime plus tokenizers against a quantised INT8 model. Anyone reading the old table would have reinstated the exact dependency that caused the outage, so the rewrite says plainly that it must not come back and why. Four packages the code does import were missing entirely (onnxruntime, tokenizers, numpy, psutil), and the counts underneath them were wrong. The seven packages that are genuinely required but never imported by name -- drivers, the server, plugins -- now have their own table explaining why grepping for imports will not find them, instead of being scattered through footnotes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Setting state in an effect body trips react-hooks/set-state-in-effect, which makes `npm run lint` exit 1. Adjusting the count during render instead also removes the frame where the browser paints the previous line's length before snapping back to zero. Same change as cc718d0 on fix/auth-secret-guard-ci, applied byte-for-byte so the two branches merge without conflict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README carried a hardcoded shields.io "build passing" badge that was never connected to anything and would have stayed green through any failure. Replace it with the GitHub Actions badge for a workflow that actually runs the suite: pytest on Python 3.13, then tsc, lint and build for the frontend. The workflow is a byte-identical copy of the one on fix/auth-secret-guard-ci so merging either branch is conflict-free. All four steps verified locally: 79 tests passed, tsc 0, lint 0, build 0. Also put the live URLs at the top, replacing a "Live demo" link that pointed at an in-page anchor showing a GIF rather than the running app. Both tiers were probed before linking them: docuqueryvc.vercel.app returns 200, and docuqueryprod.up.railway.app/health returns 200 with database: ok. The badge stays unresolved until this workflow reaches the default branch -- GitHub reads Actions badges from the default branch, not the current one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…arness
The project had no way to measure itself: no evaluation script, no benchmark,
no ground-truth data, and no document corpus. Nothing quantitative about
retrieval quality could be claimed or checked.
This adds a harness that drives the real application code -- the same parser,
chunker, embedding service and retrieval pipeline the API uses -- rather than
reimplementing any of it for measurement:
ingest_corpus.py per-stage ingestion timings, one synthetic user per
chunking config so the production per-user filter does
the isolation between configs
build_eval_set.py ground-truth queries generated from indexed passages,
anchored to (document, page) so one query set stays
valid across chunk sizes
run_retrieval_eval.py Recall@k, Precision@k, MRR, NDCG@k at document, page
and answer-span level
run_config_sweep.py chunk size, overlap, threshold and metadata filtering
compared on one fixed query set
run_perf_bench.py embedding throughput, retrieval latency percentiles,
LLM time-to-first-token
AUDIT.md records what is measurable today and, just as importantly, what is
not: retrieval quality cannot be computed until a document corpus exists, and
that is stated rather than approximated. Results in results/ are from a
four-file smoke corpus and exist to prove the code path, not to be quoted --
except the embedding batch-size sweep, which is hardware-bound and stands on
its own (batch 32 is 6.17x faster than unbatched, 3.4x faster than batch 64).
The audit also turned up four defects, recorded in AUDIT.md section 7.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
auth_secret is the only input to the session cookie's HMAC, and it fell back to "dev-only-change-me" -- a constant committed to a public repository. Any deployment that never set AUTH_SECRET was signing sessions with a key anyone could read, so anyone could mint a cookie for any user id. Nothing forced the override, and AUTH_SECRET was absent from .env.example, so there was not even a prompt to set it. A model validator on Settings now rejects an empty or still-default secret whenever debug is off. It runs at Settings() construction, which app.main performs at import, so a misconfigured process dies before create_app() wires a single route rather than serving forgeable sessions. Debug builds stay exempt so local development needs no setup. Also documents AUTH_SECRET in .env.example alongside the three other AUTH_* variables, which were equally undocumented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every test in the suite mocks the database. That means no SQL this application
issues has ever been executed by a test: not the per-user `WHERE user_id = ...`
filters that are the whole of the tenant isolation story, not the ON DELETE
CASCADE behaviour, not the pgvector column types, and not the migrations
themselves. A mock returns whatever it was told to return, so a query that
selects another user's rows passes just as happily as one that does not.
This adds a second tier rather than converting the existing one. Tests marked
`integration` run against a real PostgreSQL server with pgvector; everything
else keeps mocking and stays fast.
tests/conftest.py `migrated_database` runs Alembic to head once per session,
`db_session` truncates and hands out a session, and
`client` binds an httpx client to that same session via a
get_db dependency override, so a row a test writes is
visible to the request it then makes.
pytest.ini registers the `integration` marker so `-m integration`
and `-m "not integration"` both select meaningfully.
ci.yml adds a pgvector/pgvector:pg16 service container, matching
the Railway major version, and sets TEST_DATABASE_URL.
The tier is opt-in through TEST_DATABASE_URL. Unset -- a laptop with no
database, which is the default -- and every integration test skips, so
`pytest -q` behaves exactly as before (84 passed). CI sets it, so CI runs them.
Alembic is driven through a subprocess rather than `alembic.command`. Two
reasons: migrations/env.py calls `asyncio.run()`, which raises when reached
from inside pytest-asyncio's running loop, and it resolves the database URL
from an lru_cache'd Settings, so an in-process override would leak into every
later test in the session.
pytest-cov is added and CI prints coverage, but nothing is gated on it. A
threshold before there is a baseline would only encode today's number as a
target.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Migration a9f3d6c1b5e8 switched the embedding model from bge-base to bge-small
and cleared every vector to resize the column:
UPDATE document_chunks SET embedding = NULL
It did not touch `documents.status`. The affected documents therefore still
read `indexed` while not one of their chunks can be matched by a vector search.
The file list shows them as ready, retrieval never returns them, and the
assistant cannot find a document the user watched it ingest. The audit found
the live database in exactly this state: 5 documents at status='indexed' over
31 chunks, only 30 of which carry an embedding.
a9f3d6c1b5e8 has already run against production, so it is not edited -- Alembic
will not re-run a stamped revision, and editing it would leave every deployed
database describing a history it does not have. A new migration corrects the
state instead:
d5e2c8a7f1b3 sets status='failed' on documents that claim to be indexed but
own at least one NULL-embedding chunk, with a downgrade that
restores the previous status using the same predicate.
Re-embedding is deliberately not done in the migration. It would mean loading
the ONNX model and re-reading source files while the application waits to boot,
and railway.json runs `alembic upgrade head` on every start. 'failed' is the
honest description of these rows; repairing them is a separate, out-of-band
step:
scripts/reindex.py rebuilds one document (--document-id) or every failed
document whose file still exists (--all-failed), with
--dry-run to preview.
The script clears existing chunk rows before re-running the pipeline, because
store_chunks appends rather than replaces -- without the delete a rebuilt
document would carry two generations of chunks and double its retrieval hits.
Documents are processed serially: embedding is CPU-bound and shares the
instance with live traffic, which is how the earlier OOM crash happened.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… section 7
The audit counted 13 routes against a README API table listing 12. The missing
one is `GET /documents/{id}/debug`.
It is documented rather than removed. Nothing under frontend/ calls it, which
was the argument for deleting it, but it is scoped by `current_user.id` exactly
like every other document route, so it exposes nothing the owner cannot already
read -- and it reports precisely the mismatch that defect 2 is about: recorded
chunk count, actual chunk rows, and how many of them carry an embedding. That
makes it the check an operator runs before and after scripts/reindex.py, so
deleting it would remove the tool that verifies the fix shipped alongside it.
AUDIT section 7 is updated to match reality rather than left asserting defects
that are now closed:
1. withdrawn -- not reproducible. backend/.env.example has named bge-small
since 2f46ff0, the commit that swapped the model, and it agrees with both
Settings.embedding_model and the ONNX directory actually loaded. The
finding was almost certainly read from an untracked local .env.
2. fixed by d5e2c8a7f1b3 and scripts/reindex.py.
3. fixed by the README row above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…al SQL
Every document route and /retrieve narrows its query with
`Document.user_id == current_user.id`. That clause is the entirety of this
application's multi-tenancy, and no test had ever executed it. The existing
suite hands the routes an AsyncMock, which returns what the test told it to
return -- a route that had lost its ownership filter would have passed
identically.
Seeds two real users in PostgreSQL, gives one a document with an embedded
chunk, and asserts the other cannot reach it through any route that takes a
document id: GET /documents (absent from the list), GET /documents/{id},
/chunks, /debug, DELETE, and POST /retrieve.
Two properties worth calling out:
- A cross-tenant read returns 404, not 403, and the tests assert it. 403
would confirm the document exists, turning an opaque id into an oracle for
"does this belong to someone else".
- DELETE is checked for having done nothing, not merely for reporting 404.
A route that deleted first and then reported the row missing would satisfy
the status code alone.
A positive counterpart asserts the owner still retrieves their own chunk, so
the isolation assertions cannot be satisfied by retrieval being broken for
everyone. Query embedding is stubbed there -- loading the ONNX model tests
nothing about ownership -- while the vector search and its filter run for real.
The auth tests cover what a mock cannot: the UNIQUE constraint on users.email
that makes a duplicate signup a 409 (including a differently-cased address,
which only the real index can collide), a valid cookie failing once its user
row is deleted, and identical 401 bodies for a wrong password and an unknown
email, so the pair cannot be used to enumerate registered addresses.
Also adds the ON DELETE CASCADE check: deleting a user really does remove the
documents and chunks, rather than raising a foreign-key violation or orphaning
rows. That cascade is declared in both the ORM and the initial migration and
had likewise never been executed by a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A migration that is only ever read is not known to work, and this repository
has a specific reason to care: railway.json runs `alembic upgrade head` on
every boot, so a migration reaches production the moment a PR merges, and a
downgrade that was never executed is no way back at all.
These tests invoke the Alembic CLI against real data:
- upgrade -> downgrade -> upgrade across d5e2c8a7f1b3, seeded with the exact
state a9f3d6c1b5e8 left behind (a document marked `indexed` whose chunk has
a NULL embedding). Asserts the status flips to `failed`, comes back to
`indexed` on downgrade, and flips again on re-apply, so re-running is
deterministic rather than cumulative.
- a document with real embeddings is left untouched in both directions.
- a genuinely failed document with no chunk rows is not promoted to `indexed`
by the downgrade, which is the one way the shared predicate could have
misfired.
- chunk text and embeddings survive the round trip -- the migration rewrites
`status` and nothing else.
- `downgrade base` then `upgrade head` over the entire chain, which is the
cheapest possible guard against a downgrade that was never written.
Status is read back through raw SQL because the migration changes the row
underneath the session, and the ORM identity map would otherwise return the
stale object and pass regardless.
Every test restores the database to head in a finally block, so a failure
partway through does not strand the rest of the session on an older revision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conventions that were being restated at the start of every session, or
rediscovered the hard way, now live in one file: repository layout and why the
API layer is thin, the exact commands for the unit tier, the integration tier
and the evaluation harness, and the rules that outlast any one phase.
The rules worth stating plainly:
- One PR per phase.
- Every retrieval change ships with a before/after table against BASELINE.md.
Chunking, embedding, threshold, top-k, filtering, scoring and context
building all count. "It seems better" is not a result.
- Never edit an applied migration; correct it with a new one, write a real
downgrade, and remember that railway.json runs `alembic upgrade head` on
every boot -- merging a migration IS running it against production.
- Read frontend/AGENTS.md before touching Next.js. This is Next 16 with React
19, params is a Promise, and the bundled docs under node_modules are the
authority for this version rather than recollection.
Two operational details that have already cost time are recorded explicitly:
the integration fixtures TRUNCATE whatever TEST_DATABASE_URL points at, so the
test container uses port 5434 while the dev stack holds 5433; and AUTH_SECRET
is required whenever DEBUG is false, with the one-liner to generate one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Retrieval quality could not be measured because there was no corpus. The
existing smoke results were generated from four files, two of which were this
repository's own README, which is enough to prove the harness runs and nothing
else.
corpus/manifest.json records 27 multi-page public documents -- 3,428 pages,
~95MB -- with a URL, a SHA-256 and a licence note for each.
`download_corpus.py` fetches and verifies them; the PDFs themselves are
gitignored, since they are large and belong to their publishers. The manifest
is what makes a number reproducible: two people who run the download get
byte-identical inputs, and a document changed upstream surfaces as a hash
mismatch rather than as a quietly different score.
Source selection is narrower than planned, on purpose:
- Everything is a U.S. federal government publication (NIST x24, Federal
Reserve x3), which is public domain under 17 U.S.C. 105. That avoids
asserting redistribution rights over a third-party paper whose licence
would have to be checked item by item.
- GAO reports were the original intent and were dropped. gao.gov answers 403
to any non-browser User-Agent; it serves the same files to a spoofed one,
but working around a publisher's bot policy is not a dependency this
corpus should carry.
- arXiv was considered and rejected for the corpus proper: its default
licence is a non-exclusive distribution licence, not CC-BY, and only some
papers are CC-BY.
The resulting corpus skews toward cybersecurity, risk-management and
macroeconomic prose. That is a real limitation on how far any number computed
from it generalises, and it is recorded in the manifest and repeated wherever
the results are reported rather than left for a reader to infer.
`--write-hashes` fills in a null sha256 and records page count and embedded
title read from the file itself, so the manifest cannot drift from the bytes.
It refuses to overwrite an existing hash, so it can never silently bless a
document that changed upstream.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generated query set has a structural blind spot: build_eval_set.py writes
one question per passage, from that passage, so every query it produces is
single-hop, single-document, and answerable by construction. Three things that
matter in real use are therefore unmeasurable.
unanswerable (15) Questions the corpus cannot answer. This is the slice that
shows whether retrieval abstains or hands the model
confident-looking, topically-adjacent chunks above the
similarity threshold -- which is how a grounded assistant
ends up inventing an answer.
multi_hop (15) Questions needing two documents, to show whether top-k
surfaces both or only the more lexically similar one.
followup (15) Two-turn exchanges whose second turn ("what about the
third one?") is meaningless alone. Each carries a
reference standalone rewrite, so query condensing can
later be scored against a fixed target rather than
against itself.
Ground truth references documents by manifest id rather than database
document_id, which is regenerated on every ingest.
Marked DRAFT and deliberately not used: no number in this PR comes from it.
The caveats are recorded in the file rather than assumed away -- the items were
written from document scope, not by reading 3,428 pages, and the unanswerable
slice is the fragile one, because several NIST documents carry crosswalks to
external frameworks and a question meant to be unanswerable can turn out to be
partly covered by a mapping table.
Also corrects the evaluation commands in CLAUDE.md. The harness scripts import
`_common` flatly and rely on _common.py to put backend/ on sys.path, so they
run as paths; `python -m evaluation.ingest_corpus` raises ModuleNotFoundError.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The draft slice was written from document titles and scope, and the file said
plainly that this was the fragile part. It was: four of the fifteen questions
asked about frameworks the corpus genuinely discusses.
Extracted the full text of all 27 PDFs and searched each item's distinguishing
terms, case-insensitively and on word boundaries. Findings:
PCI DSS 18 occurrences across SP 800-92, 800-146, 800-209 and 800-218,
cited as a compliance driver for logging and storage.
HIPAA 33 occurrences, notably IR 8259 and SP 800-207.
Basel III present in both Federal Reserve annual reports, alongside
'liquidity coverage ratio'.
NBER 'National Bureau of Economic Research' appears in both annual
reports, defining the recession shading on their charts.
Those four are replaced with topics verified absent: the EU Digital
Operational Resilience Act, Brazil's LGPD, People's Bank of China reserve
requirements, and the Bank of Japan's inflation target index. Each item now
records why it is absent, including what it replaced.
Word boundaries turned out to matter more than expected. A plain substring
search reported 903 hits for 'Rust' and would have condemned a perfectly good
question: every one of them was inside the word 'trust', which a corpus
containing SP 800-207 on Zero Trust Architecture has rather a lot of. The
reverse also appeared -- 'NCSC' matches three times, but as the U.S. ODNI
National Counterintelligence and Security Center, not the UK National Cyber
Security Centre the question is about, so that item stands with a corrected
rationale.
The multi_hop and followup slices are explicitly NOT verified this way and the
file says so. Their ground truth asserts which document holds the answer, which
a term search cannot confirm; that still needs a human read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 0 — baseline: integration tier, migration fix, evaluation corpus
Groundwork that phase 8 builds on. Fourteen commits: an integration test tier against
real PostgreSQL, the
d5e2c8a7f1b3follow-up migration, a pinned 27-documentevaluation corpus with hand-written slices, the root
CLAUDE.md, and the AUTH_SECRETstartup guard.
Separately, and more urgent than this PR
DEBUG=falsewithAUTH_SECRETunset means production is already signing sessioncookies with
DEV_AUTH_SECRET = "dev-only-change-me"— a constant in a publicrepository. That is the condition the guard was written to prevent, and it is live
right now on
main, which does not have the guard. Anyone who reads the repo can minta valid session cookie for any user id.
The guard in this PR is the fix, but setting the variable is what actually closes it,
and that does not need to wait for this merge.
What is in here
tests/conftest.py, three test modules): opt-in viaTEST_DATABASE_URL, skipped entirely when unset. Exercises tenant isolation, theauth routes and the migration chain against real SQL rather than mocks.
d5e2c8a7f1b3:a9f3d6c1b5e8cleared every embedding without resettingdocuments.status, leaving rows markedindexedwhose chunks cannot be matched.This marks them
failed;scripts/reindex.pyrebuilds them out of band. Theoriginal migration is not edited, because it has already run.
hand-written unanswerable / multi-hop / follow-up slices. The unanswerable slice is
mechanically verified against the corpus text; the other two are explicitly marked
as not verified.
CLAUDE.md: the standing rules for this repository.Tests
pytest -qpasses with the integration tier enabled against pgvector/pgvector:pg16.