Retriever seam + QueryEngine, and a deepening pass over what it exposed (issue #16) - #21
Merged
Merged
Conversation
Four retrieval levers had been proven in the eval harness -- BM25 hybrid,
cross-encoder reranking, query rewriting, refusal handling -- and none could
ship. They lived under src/eval/, so activating one in production would have
meant importing the benchmarking package from the serving path.
Introduces a runtime-checkable Retriever Protocol, retrieve(query, top_k) ->
list[SearchResult], and four adapters behind it. Two conform directly
(DenseRetriever over the vector store, BM25HybridRetriever); two *compose* an
inner Retriever (RerankingRetriever over-fetches then re-scores;
MultiQueryRetriever fans rewritten queries out, unions, dedups by chunk_id).
The four levers move from src/eval/ to a core src/retrieval/ package by git mv
-- no shims -- the same dependency-direction fix as ADR 0003.
TEACHES: A seam is where you can change behaviour without editing in that
place. Choosing *where* it goes is a separate decision from what sits
behind it, and here the two adapters that compose an inner Retriever are
what prove the seam is real rather than hypothetical: one adapter is a
guess, two that stack are a contract.
TEACHES: Dependency direction is a design constraint, not a detail. Code the
product depends on must never live inside the package that measures it.
VERIFIED: Behaviour-preserving -- production does not consume the seam yet
(that is the next commit). 311 tests pass; contract tests run every
adapter through the same Protocol.
Three answer prompts had drifted apart: the sync query path used a plain one,
the streaming path a Markdown one, and the eval harness carried a third copy
with its own context join and its own chunking defaults. The harness was
scoring a pipeline that was not the one being served. Telemetry assembly was
written out at four separate sites in the backend.
Introduces src/query_engine/ as a deep module: a two-method interface (ask,
ask_stream) over a large implementation. prompt.py holds the single Markdown
answer prompt -- the sync path adopts it, which is a deliberate product change,
because the frontend renderer expects Markdown and eval must measure what
ships. telemetry.py assembles StageTelemetry once from provider-reported usage.
engine.py shares those helpers across both paths; only streaming runs the
reasoning pass, so sync keeps its single LLM call.
Retrieval is injected through the seam, selected by a new RETRIEVER_STRATEGY
config field (default "dense", behaviour-preserving). RAGBackend delegates
query / query_with_telemetry / stream_query to the engine and keeps only
conversation persistence.
TEACHES: Depth is measured at the interface, not in the implementation. Two
methods now hide prompt construction, context assembly, retrieval
composition, telemetry and an optional refusal gate -- and callers learn
two names to get all of it. That ratio is the whole point.
TEACHES: Duplication that drifts is worse than duplication that stays in sync,
and three copies of a prompt will always drift. One owner or none.
VERIFIED: test_backend.py and test_backend_telemetry.py pass unchanged -- the
facade contract is intact. 325 tests pass. Engine tests assert the sync and
streaming paths issue identical answer instructions.
An evaluation harness that scores a different pipeline than the one served is
worse than no harness: it reports numbers with confidence and they describe
something else. EvalPipeline re-implemented retrieve -> generate with its own
prompt, its own bare-newline context join (no [filename] prefix) and its own
top-k handling.
EvalPipeline now composes its levers into one Retriever behind the seam and
delegates to a QueryEngine. Its divergent copies are deleted, not deprecated.
EvalConfig's chunking, top-k and model defaults derive from src/config.py, so
there is one source of truth rather than two that agree by habit.
Accepted, deliberate consequences, recorded rather than hidden: eval telemetry
granularity collapses from per-lever stages to the engine's retrieve/generate;
the dead rewriter_cost_usd field is dropped (verified zero readers); and with
the refusal gate checked before the no-documents branch, an empty index now
returns the no-documents sentinel instead of generating from empty context.
TEACHES: When a measurement tool and the thing it measures diverge, fix the
tool by deleting its copy -- not by syncing two implementations. Syncing is
a promise; deletion is a guarantee.
TEACHES: An ADR earns its keep by recording the consequences you accepted, not
just the decision you made. Future readers need to know what you knew.
VERIFIED: ADR 0004 written; CONTEXT.md gains the vocabulary. A parity test pins
eval to the shipped prompt and context builders, so the two cannot drift
again silently. 327 tests pass; production backend tests unchanged.
Standards work on the two modules the previous commits introduced, taken
seriously rather than deferred:
QueryEngine had grown past the project's 250-line ceiling, so StreamResult, the
event alias and the streaming helpers move to src/query_engine/streaming.py.
The two `Any`s are gone: ask_stream and _stream_reasoning yield a typed
StreamEvent (str | StreamResult) and the facade narrows with isinstance plus an
invariant assert. list[dict] tightens to list[dict[str, str]].
_source_dict() is extracted so the two query paths stop writing out the
source-citation shape independently -- the sync path adds chunk_index on top,
which is exactly the kind of difference that survives when a shape has no
owner. The rewriter's expansion unpack gets a name instead of a magic [0].
Production ingestion now reads CHUNK_SIZE / CHUNK_OVERLAP from src/config.py,
set to production's actual 512/64, and the harness already derives from the
same source -- so "baseline" eval measures production's chunking.
TEACHES: A line ceiling is not bureaucracy. Hitting it is a prompt to ask which
responsibility wants to leave, and here the answer was obvious once asked.
TEACHES: `Any` is a note saying "I did not work out the type yet". Two of them
turned into a real discriminated union that the facade now narrows safely.
VERIFIED: Behaviour-preserving. 327 tests pass; production backend tests
unchanged.
Two honesty corrections to the record, made because an ADR that overstates its
own impact teaches the next reader the wrong thing about the system.
The chunking-config unification has NO behaviour delta on either side.
Production and eval both stay at 512/64; only the previously eval-only
config.CHUNK_SIZE constant was realigned to production's real values. The
original text implied a change where there was none.
Also notes the one genuinely latent change: with an empty index and no refusal
handler, an eval query now returns the no-documents sentinel instead of
generating from empty context. Latent because eval always ingests before
querying -- but latent is not the same as absent, and the difference belongs in
writing.
TEACHES: Records get corrected, not quietly rewritten. An ADR is a decision at
a date; a follow-up note that says "this said more than it should have" is
more useful than a silent edit that erases the mistake.
Importing src.llm_handler called load_dotenv() as an import side effect. On any
machine with a .env, merely importing the module armed real provider
credentials -- so the test suite made billable OpenAI calls, and failed with 7
errors on a clean checkout without a funded key. It passed only when someone
set CI_LLM_MOCK=1 by hand.
Root fix, at the cause rather than the symptom: a library import must not read
files or arm credentials. Dotenv loading becomes an explicit
src.config.load_env() that the two entry points -- the FastAPI app and the eval
CLI -- call deliberately. Everything under src/ stays credential-free on
import.
Test-side fix: the provider stub is default-on instead of opt-in. A suite must
never depend on ambient credentials and must never spend money by accident.
Opting IN to a real provider now takes RAG_QA_LIVE_LLM=1, which is the correct
direction for that switch -- the dangerous thing should require a deliberate
act, not the safe thing.
TEACHES: Import side effects are invisible coupling. Nothing at the call site
of `import x` suggests it might read your filesystem or arm an API key,
which is precisely why the bug survived so long.
TEACHES: Defaults are a safety mechanism. Ask which way round a flag should be,
and put the cost on the dangerous path.
VERIFIED: 327 tests pass with no environment flags set at all.
The user-visible one: eval run progress reported 0.0 for a run's entire
lifetime and then jumped to 1.0. A run was registered with n_total=0 -- the
question count is unknown until datasets load -- and the progress callback then
discarded the runner's `total`, so n_total stayed 0 forever and the branch
computing a fraction was unreachable. update_progress now accepts the total,
and the fraction rule moves out of the route into a tested
progress_fraction().
Note *why* it survived: both sides of that seam were covered. The rule lived in
the one place nothing could reach, which is a coverage report's blind spot.
Source citations: the sync path added chunk_index on top of the shared source
shape and the streaming path did not, so a citation's fields depended on which
endpoint produced it. _source_dict now owns the field.
Vector store: the eval pipeline reached past ChromaVectorStore into its private
collection -- twice, redundantly -- to build a BM25 corpus, contradicting the
encapsulation rationale stated in that same file. all_chunk_texts() joins the
store's interface and the reach-around is deleted.
Eval doubles: the HTTP layer imported a private _DummyLLM out of the CLI module
and repeated its environment dispatch verbatim. Both now use one public
src/eval/doubles.py.
TEACHES: Coverage measures lines executed, not rules tested. A rule with no
reachable home is untested no matter how green the report looks -- move it
somewhere a test can address it.
TEACHES: When code reaches past an interface into a private attribute, the
interface is missing a method. Add the method; do not widen the access.
VERIFIED: 345 tests pass.
SearchResult -- the type the whole Retriever seam is defined in terms of --
lived in src/vector_store.py, the module that does `import chromadb`. Ten
modules therefore imported a storage vendor merely to name what a Retriever
returns, including src/retrieval/base.py, whose entire job is to declare the
Protocol. Document and Chunk had the same shape of problem one step earlier:
naming a chunk meant importing the file-parsing module and its optional pypdf,
python-docx and BeautifulSoup branches.
All four now live in a leaf src/domain.py that imports nothing from this
package. content_hash is byte-identical to the previous _hash_text -- verified
against it before the move, non-ASCII included -- because those ids are
persisted in SQLite and ChromaDB, and a changed hash orphans every stored row.
Separately: ChromaVectorStore's distance -> similarity conversion is only
correct in cosine space, and nothing enforced it. `metadata={"hnsw:space":
"cosine"}` was written out at nine construction sites, where forgetting it
produces silently wrong scores rather than an error. ChromaVectorStore.open()
now owns the setting and every site goes through it.
TEACHES: Dependencies point toward stability. A leaf module with no imports is
the only place a shared vocabulary can live without dragging vendors behind
it -- and a test that imports src/domain.py in a subprocess pins that.
TEACHES: An invariant repeated at N call sites is an invariant with no owner.
Move it behind a constructor and the wrong version stops being reachable.
VERIFIED: Recorded as ADR 0005; CONTEXT.md gains the domain entry. Hash parity
checked against the old implementation before anything moved. 356 tests
pass.
The eval runs directory was a module-level constant bound at import time. To
configure it, the CLI reassigned another module's global
(_storage.EVAL_RUNS_DIR = ...) at four call sites, and three test modules each
carried their own copy of a fixture that set an env var and then
importlib.reload()ed storage so it would notice. Five WHY-comments across three
source files existed to explain the workaround.
storage.runs_dir() now resolves per call and every read and write takes
base_dir=, so callers inject instead of mutating. The workaround, its
explanatory comments, and two of the three duplicated fixtures are gone; the
survivor moved to conftest.
Also single-sourced the literals the review found duplicated. rerank_top_n and
final_top_k in the eval config were independent numbers that happened to equal
production's, with a comment asserting they matched and nothing enforcing it.
The refusal threshold and its user-facing no-answer text lived only in the eval
config -- a product string inside a benchmarking schema -- so whoever wired the
gate for production would have picked a second threshold by hand.
TEACHES: When code needs a comment explaining a workaround, the workaround is
the bug. Five comments pointing at one design flaw is the design flaw
asking to be fixed.
TEACHES: "These two numbers happen to be equal" is not a guarantee, it is a
coincidence with a countdown on it.
VERIFIED: Environment reads outside config: 15 down to 8, each with one owner.
363 tests pass.
ADR 0004 put every strategy behind one seam but left the rule for how adapters
*stack* in two modules, selected by two vocabularies: production by a strategy
string, eval by four boolean levers. Eval also derived the effective top-k from
the composition -- when reranking is on, the final count is final_top_k because
the reranker over-fetches a wider set first -- and production had no equivalent
of that rule at all, passing TOP_K_RESULTS flat.
The two agree today only because final_top_k (5, in all six shipped configs)
and TOP_K_RESULTS (5) happen to be the same number. Agreement by coincidence,
not by construction: tuning either one would have made the harness measure a
different pipeline than the one served -- the exact failure ADR 0004 exists to
prevent, one level up from the prompt. Nothing would have caught it. The parity
test uses a non-reranked config, and the rule itself had no test because it was
a private method on an object whose construction needs a live Chroma client and
an 80MB cross-encoder download.
compose_retrieval() now owns the rule and returns a frozen RetrievalPlan
carrying the retriever and its top-k together, so a caller cannot obtain one
without the other. It takes an already-built base Retriever rather than a
vector store -- which is precisely what makes it testable without that I/O.
Production's strategy names become presets over it via build_retrieval_plan().
src/retrieval/factory.py is deleted rather than shimmed, per precedent.
TEACHES: If two values must travel together to be correct, return them
together. A type can make the incorrect combination unrepresentable, which
beats a comment asking callers to remember.
TEACHES: "Untestable" usually means "takes its dependencies as concrete things
instead of as arguments". Change the parameter, not the test.
VERIFIED: Recorded as ADR 0006. hybrid and multi_query stay deferred exactly as
ADR 0004 decided. 377 tests pass.
Submitting an eval run was reachable only through FastAPI. The route handler
owned config resolution, run-id and git-SHA derivation duplicated from
EvalRunner, an import of a private _DummyLLM out of the CLI module, the
registry lifecycle, and a 50-line background worker -- while the module
actually named services/eval_runs.py held only bookkeeping, a dict behind a
mutex, and never imported the eval package. The split was inverted.
The cost was testability, and it compounded. Because nothing could call the
orchestration directly, its tests had to boot a TestClient and inject a fake
LLM through an environment variable -- even though EvalRunner accepts one as a
parameter. The failure path had no coverage at all, and the runner/registry
joint that dropped the progress total (fixed two commits ago) went untested by
construction.
src/eval/submission.py now owns it. The progress sink is a runtime-checkable
Protocol rather than the concrete registry, so the eval package never imports
the web layer; RunRegistry satisfies it as written, and a test asserts exactly
that. run_id_override is gone -- it existed only to reconcile a run id derived
in two places from two independent datetime.now() calls that had to agree to
the second. Submission reserves the id once and passes it in.
TEACHES: When a test needs an HTTP client to exercise business logic, the logic
is in the wrong module. The awkward test is the diagnosis, not the problem.
TEACHES: Depend on a Protocol you declare, not on the concrete class across the
seam -- that is what keeps the dependency arrow pointing the right way.
VERIFIED: The route is down to HTTP translation, 478 -> 409 lines. 386 tests
pass, including the failure path, without FastAPI.
Pinned BEFORE extracting anything, so the extraction that follows can be shown
to preserve behaviour rather than merely to compile. These two clusters were
68% of RAGBackend's code with no collaborator behind them, and the least
covered in the repository: evaluation (154 code lines) had no direct tests at
all, and conversation behaviours were exercised only incidentally.
What is now pinned, in the order it will need to survive:
- realtime faithfulness persists a score, and a judge failure returns the zero
sentinel instead of propagating (the streaming endpoint calls it)
- evaluate_message scores all three metrics; skips faithfulness when the
realtime path already scored it, because re-running would duplicate the row
and skew aggregations; skips it when there are no contexts; and takes the
question from the preceding user message
- _auto_title replaces only the "New Chat" placeholder, truncating on a word
boundary
- _save_message returns an id usable after commit, persists sources, and bumps
the parent conversation
- search_conversations returns a conversation once when it matches on both
title and body
- list_conversations puts pinned conversations first
Writing them surfaced the real judge contracts, which no documentation stated:
answer_relevancy returns a 2-tuple while faithfulness and context_precision
return 3-tuples, and `details` is a JSON string rather than a dict.
TEACHES: Characterization tests describe what the code does, not what it should
do. That is the point -- they are the safety net for a refactor, so they go
in first, against the old code, and must pass unchanged afterwards.
TEACHES: Writing tests for code you are about to move is also how you discover
its real contract. Two surprises here would have become bugs later.
VERIFIED: 33 tests pass across the two files, against the unmodified backend.
The evaluation cluster was 154 code lines inside a 1265-line facade with no
collaborator behind it -- which is exactly why its skip and dedup branches had
no direct tests. Apply the deletion test: removing this code would not remove
the complexity, it would relocate it to the route handler, where
evaluate_message alone would become the largest function in the API layer. So
it is real work sitting in the wrong module, not a pass-through to delete.
src/evaluation.py becomes a package: judges.py holds the three pure scoring
functions, message_evaluator.py the orchestration around them. That also
resolves a naming collision -- a module named src/evaluation.py sat beside
src/eval/, src/api/routes/evaluation.py and src/api/routes/eval.py, was shared
by production and the harness, and every reader's first guess about it was
wrong.
The three near-identical "skip if already scored" blocks collapse into one
per-metric step, preserving the difference the characterization tests pinned:
faithfulness carries `details` on the cached branch because the frontend
renders a claim-level breakdown from it; the other two do not.
The judges are injected via a Judges struct defaulting to the real ones.
Substituting a judge used to mean reassigning a module global; it is now part
of the interface, and the tests monkeypatch nothing.
TEACHES: The deletion test distinguishes a shallow wrapper from a deep module
in the wrong place. If complexity vanishes, it was a pass-through; if it
reappears across N callers, it was earning its keep -- move it, don't
inline it.
TEACHES: If faking a dependency requires reaching into module internals, the
dependency is not part of the interface yet. Make it a parameter.
VERIFIED: The facade keeps all three public methods, so routes are untouched
and the characterization tests pass unchanged.
RAGBackend was 1265 lines. Of its ~525 code lines, 68% belonged to two clusters
with no collaborator behind them -- conversation persistence (204) and
evaluation orchestration (154) -- written as inline SQLModel queries. 17 of the
19 select() calls in all of src/ were in this one file.
The facade was not shallow, and that distinction drove the whole approach.
Per-cluster the deletion test says complexity reappears rather than vanishes:
conversation persistence would spread across nine route handlers. It was real
work in the wrong module, and the price was testability -- reaching any
conversation behaviour meant constructing a Chroma collection, three LLM
handlers, a retriever and a query engine first.
Now src/conversations/ (ConversationStore, ConversationHistory, and the wire
shapes that had been written out at five call sites) and src/evaluation/
(judges plus MessageEvaluator). Both take a session factory rather than an
engine, so the one-session-per-operation policy keeps a single owner.
Behaviour was pinned before it moved: the characterization tests went in first,
then the extraction was made to pass them with only monkeypatch paths changed.
Also adopts the dependency-injection seam everywhere. BackendDep moves into
src/api/dependencies.py and all six route modules use it. Previously one module
used the seam and five read request.app.state.backend directly -- so overriding
get_backend in a test changed the behaviour of one route module out of five,
which is the kind of half-adopted pattern that is worse than none.
TEACHES: Extract along the lines the code already draws. These two clusters
shared no state with each other and changed for different reasons -- the
seams were already there, just unmarked.
TEACHES: A pattern adopted at one call site out of six is not a pattern, it is
a trap for whoever assumes it holds.
VERIFIED: backend.py 1265 -> 783 lines. Recorded as ADR 0007.
tests/test_conversations.py covers the same ground directly: 33 tests in
0.84s against an in-memory database. 436 tests pass.
Found by writing a test for a branch that only became reachable once its
manifest path stopped being a literal at the call site -- which is the point of
the second half of this commit.
The ingestion bug is real and user-facing. Chunk ids are content-addressed, so
a document containing the same text twice -- a repeated boilerplate footer, a
disclaimer page, a CSV with duplicate rows -- produced the same id twice within
one upsert batch. ChromaDB rejects such a batch with DuplicateIDError, so the
entire upload failed rather than storing the document. Reproduced through the
production path (RAGBackend.ingest_file), not just against the store.
Fixed in ChromaVectorStore.upsert, which already owns the idempotency contract:
ids repeated inside a single batch collapse to their first occurrence. Two
chunks with the same content-addressed id ARE the same chunk, so collapsing
them is what the id scheme already means. One fix, every caller.
Also from the review's defect list:
- The spend ceiling is the harness's only guard on real money and had no test,
because it was written inline inside the per-question loop where nothing
could reach it. Extracted as assert_within_spend_ceiling with a named
SpendCeilingExceeded, covered including the boundary (spending exactly the
budget is allowed; strictly over aborts).
- _ingest_ml_papers hardcoded its manifest path, so the only branch a test
could take was the missing-manifest no-op. The path is now a field, and the
58-line ingest branch has tests.
- database.get_session's docstring claimed to be how routes obtain sessions.
No route uses it. Said so.
TEACHES: Fix a bug where the invariant lives. The store owns idempotency, so
the store owns duplicate collapse -- fixing it in the caller would have
left every other caller broken.
TEACHES: Hardcoded paths and inline rules are untestable by construction. Make
the value a parameter and the branch becomes reachable -- which is how the
crash above was found in the first place.
VERIFIED: 449 tests pass, including a regression test that ingests a document
with a repeated footer end to end.
document_loader.py was 504 lines -- twice the project's per-module ceiling --
holding two responsibilities that shared no code. DocumentLoader never called
TextChunker and vice versa; only the value types crossed between them. They
also change for entirely different reasons: adding a format touches parsing,
tuning retrieval quality touches chunking.
Format dispatch was a dict of private methods bound to self, rebuilt on every
call, gated by a separate hardcoded extension set kept in step by hand. Nothing
could be registered from outside or called on its own -- and the coverage hole
landed exactly where the risk was. PDF, DOCX and HTML, the three formats with
an optional dependency and an ImportError fallback, had zero tests.
Now src/ingestion/: parsers.py (one function per format in a PARSERS registry,
with SUPPORTED_EXTENSIONS derived from it via frozenset(PARSERS), so the two
cannot disagree), loader.py (paths, source metadata, batch error policy, no
format knowledge), chunking.py (strategies and quality filters).
The PDF line-break and hyphen normalisation -- the module's most valuable
logic, whose docstring names the bug it fixes -- is lifted out as a pure
str -> str function and now has seven direct tests, including the case that
separates a wrapped word ("develop- ment") from a real compound
("self-attention").
Also closed the rest of the review's untested list: the dot-leader
table-of-contents filter, the minimum-length floor and its boundary,
_apply_word_overlap's word-boundary guarantee, the semantic strategy that no
code constructed, the chunker's three validation errors, and the vector store's
two query guards plus the query_text branch production actually takes.
TEACHES: Derive one thing from the other and they cannot drift. A registry the
extension set is computed from beats two lists someone has to sync.
TEACHES: Pure functions are where the valuable logic wants to live. The PDF
normaliser was the highest-risk code in the module and untestable inside a
private method; as a str -> str function it took seven tests and minutes.
VERIFIED: Recorded as ADR 0008. 508 tests pass.
CLAUDE.md names ruff, black, mypy and radon as this project's quality bar, but
no configuration recorded what that bar actually was -- so the standard was a
sentence in a document rather than something anyone could run. The first ruff
pass over the repository produced 231 findings.
Most were real, and were precisely what the stated standard already asks for:
46 dead imports, 46 unsorted import blocks, and 60-odd pre-PEP-585 annotations
(List[x], Optional[x]) that CLAUDE.md explicitly forbids.
Ruff also caught a genuine bug in this session's own work: four spend-ceiling
tests written as bare `f(...) is None` expressions with no `assert`, so they
executed the call and asserted nothing. A test that cannot fail is worse than
no test, because it reports safety it does not provide.
Also fixed: six HTTPException translations that dropped the cause chain (now
`raise ... from exc`), an import stranded below module code in upload.py, an
unused local, a mutable class attribute now ClassVar, six zip() calls given an
explicit length policy, and two Protocols this session had redeclared locally
instead of importing -- an identical local copy is a *different type* to a
checker, so a real QueryRewriter failed to type-check through the composition
module.
pyproject.toml now records the rule set (E, W, F, I, B, UP -- the families that
map onto stated standards) with a written reason for each deliberate exception,
so exceptions are not re-argued on every run. Opinion-heavy families are left
out on purpose: a linter nobody can get to zero is a linter everybody learns to
ignore.
TEACHES: A standard nobody can execute is a preference. Put it in config, get
it to zero, and it becomes a property of the repository.
TEACHES: Linters find bugs, not just style. The assertion-less tests were mine,
written in this same session, and no amount of care caught them by reading.
VERIFIED: Ruff 231 -> 0. 508 tests pass.
Three defects that 514 green tests could not catch, because nothing in the
suite had ever started the process or read the documentation.
`python -m src.api.main` started nothing. The README and CLAUDE.md have
documented it as *the* local-dev command for months, but the module had no
`__main__` guard: running it imported the app and exited 0, silently. Only
Docker ever started the server, by invoking uvicorn directly. Adds the runner,
plus a test asserting it calls uvicorn on the configured host and port.
docker-compose.prod.yml shipped with CORS wide open. allowed_origins() falls
back to ["*"] when ALLOWED_ORIGINS is unset -- deliberate, for local dev
against a Vite server on another port -- and the production compose file never
set it, while the function's own docstring claimed it did. Now pinned to the
nginx origin that serves the frontend, overridable for other deployments.
Architecture.md described a codebase that no longer exists. CLAUDE.md calls it
"the source of truth for component boundaries, data flows, and design
rationale"; it still documented src/document_loader.py, src/evaluation.py, CORS
allowing all origins, CHUNK_SIZE=500, DEFAULT_MODEL=glm-5.1, a four-variable
environment table, and none of src/domain.py, src/ingestion/, src/retrieval/,
src/query_engine/ or src/conversations/. The overview diagram, both pipelines,
every component section, the endpoint table, the environment table, the testing
table and the design decisions now match the code. README's file tree and the
"reranking is not yet wired" note follow.
TEACHES: A green test suite proves the code does what the tests say, not that
the program runs. Start it once; the things only a real process exercises
are the things only a real process will catch.
TEACHES: Documentation the team declares authoritative is part of the system.
When it drifts, it is not stale prose -- it is a broken component that no
tool will flag.
VERIFIED: Ran it. uvicorn starts clean, 27 operations registered, /health and
the four list endpoints return 200, and a live upload -> list -> delete
round-trip of a document with repeated text succeeds end to end.
Every test that entered `with TestClient(app)` ran the real lifespan, which
opens data/rag.db and data/chroma/ -- the developer's own persistent stores.
Confirmed by mtime: a single route test rewrote data/chroma/chroma.sqlite3. The
tests that swapped in a mock backend did so only after startup had already
returned, far too late to prevent it.
A session-scoped autouse fixture now rebinds the paths main.py copied from
config at import time, so the lifespan builds throwaway stores under tmp.
Autouse rather than opt-in, deliberately: the failure is silent and its blast
radius is the user's own files, so a test that forgets to opt in is exactly the
case that corrupts them.
Also adds the one endpoint missing from Architecture.md's tables
(GET /api/eval/runs/{id}/results/{question_id}), found by diffing the tables
against the live route table programmatically rather than by reading.
TEACHES: Tests are code with side effects. "It passed" says nothing about what
it touched on the way -- check, because a suite that quietly mutates real
state fails in a way no assertion reports.
TEACHES: Choose defaults by blast radius. Opt-in safety protects the people who
remember; opt-out safety protects everyone.
VERIFIED: mtime on data/rag.db and data/chroma/chroma.sqlite3 is unchanged
across a full run, and a test asserts the invariant directly.
The repository had never been black-formatted, so pyproject.toml recorded a
line-length while the tree stayed unformatted -- a standard that could be
stated but not checked.
This is that one deliberate pass: 83 of 138 files, formatting only. No
behaviour, no logic, no renames. Kept as its own commit precisely so it can be
skimmed and dismissed as exactly that, and so it never buries a real diff
underneath it.
TEACHES: Mechanical changes belong in mechanical commits. Mixing a reformat
into a behavioural change makes the review impossible and the blame useless
-- and "I'll just format the files I touched" is how a tree stays half
formatted for years.
VERIFIED: `black --check`, `ruff check` and 515 tests all green afterwards.
36 errors to 0, from four root causes -- each fixed at the cause.
The SQLModel descriptor gap (9 errors). `Model.field.desc()`, `.contains()` and
`.in_()` made the checker see the field's *value* type; a datetime has no
`.desc()`. `sqlmodel.col()` is the documented answer and reads no worse at the
call site. This had previously been written off as "the known SQLModel typing
gap" and deferred -- it had a real fix all along, which is the lesson.
Optional SDK imports (3). Three identical try/except blocks in which
`import x as _x` rebound an already-annotated name -- a genuine second binding,
not a checker quirk. One `_optional_module()` helper replaces all three, which
also removes the duplication the DRY rule asks about.
The WebSocket stream bridge (16 errors, one cause). An `object()` sentinel
passed to `run_in_executor(None, next, gen, sentinel)` widened the result to
`object`, so `(event_type, data)` could not unpack and the entire event
dispatch lost its types. A named `_next_event()` returning
`BackendStreamEvent | None` keeps the shape, and RAGBackend.stream_query gains
the return annotation it never had. With `data` typed, the three
string-payload branches differ only by the label they echo -- so they collapse
into one, and unknown labels are still dropped, which the old per-label
branches did by accident and this now does on purpose.
The dataset-name literal (1). A `DatasetName` alias lets the config and the
runner key the same closed set instead of disagreeing about it.
One exception remains, scoped and written down in pyproject.toml:
`attr-defined` is off for `src.llm_handler.adapters.*`, where the injected SDK
client is honestly typed `object`. Two better-looking fixes were tried first --
real SDK types under TYPE_CHECKING, and structural Protocols for the client and
its whole response tree -- and both are recorded there with why they fail.
Config in one place beats `Any` in a signature or six `# type: ignore`
comments, because it names the boundary instead of hiding it.
TEACHES: "The library's types are bad" is usually wrong. Three of these four
had a documented fix; only one was a genuine boundary.
TEACHES: A type error is often a design smell wearing a disguise. The sentinel
that erased the event type was also what let three duplicated branches hide
-- fixing the type simplified the code.
VERIFIED: mypy clean over 86 source files. 515 tests pass; ruff and black
clean.
elkaix
force-pushed
the
feat/step4-retriever-queryengine
branch
from
September 10, 2026 15:58
97a7125 to
7d4323d
Compare
elkaix
changed the title
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.
Closes the step-4 architecture work from issue #16, plus a full deepening pass
over the friction that work exposed.
What this branch does
Step 4 — the seam and the engine (5 commits). A
RetrieverProtocol everyretrieval strategy hides behind, four adapters over it, and a deep
QueryEngineowning retrieve → generate for both the sync and streaming paths.The eval harness converges onto the same engine, so it measures the pipeline
that actually ships rather than a second implementation of it. ADR 0004.
The deepening pass (16 commits). Six candidates from an architecture
review, each implemented and recorded:
src/domain.py)RetrievalPlan)conversations/+evaluation/(1265 → 783 lines)PARSERSregistry, chunking as its own moduleSix defects fixed that the review did not name
Four of these are user-visible; none were caught by the test suite as it stood.
.env—load_dotenv()ran as an import side effect. Red on a clean checkout.jumped to 1.0. The fraction rule lived where no test could reach it.
boilerplate footer, a disclaimer page, a CSV with duplicate rows. Content-
addressed chunk ids collided within one batch and Chroma rejected the whole
upload.
python -m src.api.mainstarted nothing. Documented as the local-devcommand in both the README and CLAUDE.md; the module had no
__main__guard, so it imported the app and exited 0, silently.
docker-compose.prod.ymlshipped with CORS at*, while theconfiguration function's docstring claimed production pinned it.
data/directory — everyTestClient(app)entered the real lifespan. Confirmed by mtime.Items 4–6 were found by starting the process. 508 green tests had never
executed the FastAPI lifespan.
Quality bar
backend.pypyproject.tomlnow records the rule set with a written reason for everydeliberate exception, so the standard is executable rather than aspirational.
Verification
Full suite green,
ruff/black/mypyclean, CI green, and the applicationbooted and exercised end to end: 27 operations registered,
/healthand thelist endpoints returning 200, and a live upload → list → delete round-trip of a
document with repeated text (the crash in item 3).
Notes for the reviewer
TEACHES:section, since this is a portfolio repoand the history is part of the artifact.
docker-compose.prod.ymlnow setsALLOWED_ORIGINS, defaulting to the nginxorigin. A deployment on any other host must override it.
hybridandmulti_queryretrieval remain deferred exactly as ADR 0004decided;
hybridneeds a BM25 corpus kept in sync with ingestion/deletion,which is a feature rather than a refactor.