Retriever seam + QueryEngine, and a deepening pass over what it exposed (issue #16)

CONTEXT.md

@@ -31,3 +31,45 @@ behaviour behind a small interface), **seam** (a boundary you can substitute at)
   telemetry assembly and the eval harness use one source of truth; the eval
   package imports from here, never the reverse. See
   [ADR 0003](docs/adr/0003-telemetry-ownership.md).
+- **domain** (`src/domain.py`) — the leaf module holding the value objects that
+  cross module seams: `Document`, `Chunk`, `SearchResult`, and `content_hash`.
+  It imports nothing from this package, so naming a type at a seam never drags
+  an implementation along. `SearchResult` used to live in `src/vector_store.py`
+  (which does `import chromadb`), so the whole `retrieval` and `query_engine`
+  packages imported the storage vendor merely to name what a Retriever returns.
+  See [ADR 0005](docs/adr/0005-domain-value-types.md).
+- **ingestion** (`src/ingestion/`) — `parsers` (one function per format behind a
+  `PARSERS` registry, from which `SUPPORTED_EXTENSIONS` is derived, plus the pure
+  `normalise_pdf_text`), `loader` (paths, source metadata, batch error policy),
+  and `chunking` (the three strategies and the quality filters). Replaces the
+  504-line `document_loader` module, whose format dispatch went to private
+  methods and whose PDF, DOCX and HTML paths had no tests. See
+  [ADR 0008](docs/adr/0008-ingestion-parsing-seam.md).
+- **Retriever** — the seam (Protocol) every retrieval strategy hides behind:
+  `retrieve(query, top_k) -> list[SearchResult]`. Implementations either conform
+  directly (`DenseRetriever`, `BM25HybridRetriever`) or *compose* an inner
+  Retriever (`RerankingRetriever` over-fetches then cross-encodes;
+  `MultiQueryRetriever` fans rewritten queries out and dedups). Live in
+  `src/retrieval/`; composed for both production and eval by
+  `compose_retrieval` (`src/retrieval/composition.py`). See
+  [ADR 0004](docs/adr/0004-retriever-seam-and-query-engine.md).
+- **QueryEngine** (`src/query_engine/`) — the deep module owning retrieve→generate
+  for both the sync (`ask`) and streaming (`ask_stream`) paths: one Markdown
+  answer prompt, filename-prefixed context, an optional refusal gate, and
+  telemetry assembly — all in one place. Both `RAGBackend` and the eval harness
+  call it, so eval measures the shipped pipeline. See
+  [ADR 0004](docs/adr/0004-retriever-seam-and-query-engine.md).
+- **ConversationStore** / **ConversationHistory** (`src/conversations/`) — the
+  two modules owning chat-thread persistence. The store handles the thread
+  lifecycle, search, export and share tokens; the history handles message
+  writes, the completed-pairs sliding window fed to the next prompt, and the
+  auto-title rule. Both take the session factory and nothing else. Wire shapes
+  live in `shaping.py`. See [ADR 0007](docs/adr/0007-backend-split.md).
+- **MessageEvaluator** (`src/evaluation/`) — orchestration around the judges:
+  load a persisted message, find the question it answered, score what has not
+  been scored yet, persist. The pure scoring functions live beside it in
+  `judges.py`. Judges are injected via a `Judges` struct so they can be
+  substituted without patching a module. See [ADR 0007](docs/adr/0007-backend-split.md).
+- **RefusalHandler** — an answerability gate (not a Retriever): refuses when the
+  top-1 similarity is below a threshold (or nothing was retrieved). Applied
+  inside the QueryEngine; off by default in production.

README.md

@@ -289,28 +289,58 @@ The `/api/chat` endpoint streams responses through structured JSON events:
 ```
 src/
 ├── api/
-│   ├── main.py                  # FastAPI app with lifespan, CORS, routers
-│   ├── models.py                # Pydantic v2 request/response schemas
-│   ├── dependencies.py          # Dependency injection helpers
-│   └── routes/
-│       ├── upload.py             # File upload + validation
-│       ├── query.py              # REST query + WebSocket streaming
-│       ├── documents.py          # Document CRUD + chunk inspection
-│       ├── conversations.py      # Conversation CRUD + search/export/share
-│       └── evaluation.py         # On-demand evaluation endpoints
+│   ├── main.py                   # FastAPI app with lifespan, CORS, routers
+│   ├── models.py                 # Pydantic v2 request/response schemas
+│   ├── dependencies.py           # BackendDep — the one DI seam for routes
+│   ├── routes/
+│   │   ├── upload.py             # File upload + validation
+│   │   ├── query.py              # REST query + WebSocket streaming
+│   │   ├── documents.py          # Document CRUD + chunk inspection
+│   │   ├── conversations.py      # Conversation CRUD + search/export/share
+│   │   ├── evaluation.py         # On-demand per-message evaluation
+│   │   └── eval.py               # Eval-harness runs, configs, compare
+│   ├── schemas/                  # Eval + telemetry response models
+│   └── services/eval_runs.py     # In-process eval run registry + progress
 ├── models/
 │   ├── conversation.py           # Conversation table (cascade relationships)
 │   ├── message.py                # Message + MessageSource tables
 │   ├── document.py               # DocumentRecord metadata
 │   └── evaluation.py             # MessageEvaluation scores
-├── backend.py                    # RAGBackend — stateful orchestration facade
+├── ingestion/
+│   ├── parsers.py                # PARSERS registry — one function per format
+│   ├── loader.py                 # Path → Document, via the registry
+│   └── chunking.py               # TextChunker — three strategies + filters
+├── retrieval/
+│   ├── base.py                   # The Retriever Protocol (the seam)
+│   ├── dense.py, hybrid.py       # Adapters: vector search, BM25 hybrid
+│   ├── reranker.py               # Cross-encoder reranking adapter
+│   ├── query_rewriter.py         # Multi-query rewriting adapter
+│   ├── refusal_handler.py        # Answerability gate
+│   └── composition.py            # The single composition rule → RetrievalPlan
+├── query_engine/
+│   ├── engine.py                 # QueryEngine — retrieve → generate, both paths
+│   ├── prompt.py                 # The single answer prompt + context assembly
+│   ├── streaming.py              # Streaming event protocol
+│   └── telemetry.py              # Per-stage telemetry assembly
+├── conversations/
+│   ├── store.py                  # ConversationStore — CRUD, search, share
+│   ├── history.py                # Sliding window, message saves, auto-title
+│   └── shaping.py                # Pure ORM-row → response-dict functions
+├── evaluation/
+│   ├── judges.py                 # RAGAS-inspired LLM judges (3 metrics)
+│   └── message_evaluator.py      # Per-message scoring + persistence
+├── llm_handler/
+│   ├── __init__.py               # LLMHandler — provider routing + fallback
+│   ├── providers.py              # Prefix → provider, credential resolution
+│   └── adapters/                 # OpenAI-compatible, Anthropic, Ollama, dummy
+├── eval/                         # Offline eval harness (datasets, metrics, CLI)
+├── telemetry/                    # Model pricing + token counting (core)
+├── domain.py                     # Document, Chunk, SearchResult, content_hash
+├── backend.py                    # RAGBackend — the orchestration facade
 ├── config.py                     # Centralized configuration constants
 ├── database.py                   # SQLite + SQLModel setup
-├── document_loader.py            # Multi-format parser (7 file types)
-├── llm_handler.py                # Multi-provider LLM adapter with streaming
-├── vector_store.py               # ChromaDB wrapper (embeddings + search)
-├── evaluation.py                 # RAGAS-inspired scoring (3 metrics)
-└── generator.py                  # Prompt templates + context assembly
+├── observability.py              # OpenTelemetry → Phoenix, fail-quiet
+└── vector_store.py               # ChromaDB wrapper (embeddings + search)
 
 frontend/src/
 ├── pages/

docker-compose.prod.yml

@@ -21,6 +21,13 @@ services:
       - OPENAI_API_KEY=${OPENAI_API_KEY:-}
       - ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:-}
       - GLM_API_KEY=${GLM_API_KEY:-}
+      # SECURITY: unset, src/config.py's allowed_origins() falls back to "*",
+      #           which is the right default for local dev against a Vite
+      #           server on another port and the wrong one for a deployed API.
+      #           nginx serves the frontend from :3000 and proxies /api/, so
+      #           that origin is the only one a browser needs. Override with
+      #           ALLOWED_ORIGINS=https://your.domain when deploying elsewhere.
+      - ALLOWED_ORIGINS=${ALLOWED_ORIGINS:-http://localhost:3000}
     healthcheck:
       test: ["CMD", "curl", "-f", "http://localhost:8001/health"]
       interval: 10s

docs/adr/0004-retriever-seam-and-query-engine.md

@@ -0,0 +1,104 @@
+# ADR 0004 — Retriever seam and the shared QueryEngine
+
+- **Status:** Accepted
+- **Sequencing:** Step 4 of the RAG architecture deepening spec ([issue #16](https://github.com/elkaix/rag-document-qa/issues/16)); resolves the Retriever-seam and shared-QueryEngine map tickets.
+- **Date:** 2026-07-12
+
+## Context
+
+Two coupled problems, both rooted in there being no seam between retrieval and
+generation:
+
+1. **Proven retrieval levers couldn't ship.** Hybrid BM25, cross-encoder
+   reranking, query rewriting, and refusal handling existed only in
+   `src/eval/`, with no production interface to activate them.
+2. **Three diverged answer prompts, and eval measured a different pipeline.**
+   The sync query path used a *plain* answer prompt; the streaming path used a
+   *Markdown* one; the eval harness carried a *third* copy ("Answer **the**
+   question… say so **clearly**") and joined context with a bare newline join
+   (no `[filename]` prefix) and different chunking defaults (512/64 vs 500/50).
+   Eval therefore scored a pipeline that was not the one served, and telemetry
+   assembly was duplicated across four backend sites.
+
+## Decision
+
+**A `Retriever` seam.** A runtime-checkable Protocol —
+`retrieve(query, top_k) -> list[SearchResult]` — with adapters that either
+conform directly or compose an inner Retriever:
+
+- `DenseRetriever` wraps the vector store (the default).
+- `BM25HybridRetriever` conforms directly.
+- `RerankingRetriever` composes an inner Retriever: over-fetches, then
+  re-scores with a cross-encoder.
+- `MultiQueryRetriever` composes an inner Retriever: fans rewritten queries out,
+  unions, and dedups by chunk_id keeping each chunk's best score.
+
+The four eval-proven levers were **promoted from `src/eval/` to a core
+`src/retrieval/` package** (via `git mv`, no shims — same dependency-direction
+fix as ADR 0003), so production can activate them without importing eval.
+
+**A deep `QueryEngine` module** owns retrieve→generate for both paths behind a
+small interface (`ask` sync, `ask_stream` streaming). The two are separate
+methods sharing prompt/context/telemetry helpers — only streaming runs the
+planning pass, so sync keeps its single LLM call. The engine owns:
+
+- The **single answer prompt**: the Markdown one, for every path (the sync path
+  adopts it — a deliberate, product-improving change: the frontend renderer
+  expects Markdown, and eval must measure the shipped prompt).
+- **Filename-prefixed context** everywhere (the eval bare join is retired).
+- **Telemetry assembly** once, from provider-reported `Usage` (ADR 0003).
+- An **optional refusal gate**, off by default. The gate is checked *before* the
+  no-documents branch, so an empty retrieval is itself an answerability signal
+  the gate may act on.
+
+**Production selects a strategy by config** (`RETRIEVER_STRATEGY`, default
+`dense`) through a `build_retriever` factory: `dense` and `reranked` are wired;
+`hybrid` and `multi_query` are recognised but **deferred** (see Consequences).
+`RAGBackend` delegates `query`/`query_with_telemetry`/`stream_query` to the
+engine and owns only conversation persistence.
+
+**The eval harness converges onto the engine.** `EvalPipeline` composes its
+levers into one Retriever behind the seam and delegates retrieve→generate to a
+`QueryEngine`; its divergent prompt/context/top-k copies are deleted.
+`EvalConfig` chunking/top-k/model defaults now derive from `src/config.py`
+(single source of truth) — production ingestion reads the same constants. The
+values are unchanged (config holds production's actual 512/64), so this is pure
+single-sourcing with no behaviour delta on either side. A parity test pins eval
+to the shipped prompt and context builders.
+
+## Consequences
+
+- **Behaviour preserved for production:** `test_backend.py` and
+  `test_backend_telemetry.py` pass unchanged — the facade contract
+  (`query`/`query_with_telemetry`/`stream_query` shapes, the streaming event
+  protocol, the empty-store path) is intact. The sync answer becoming Markdown
+  is invisible to those tests (dummy LLM) and is the intended product change.
+- **Streaming persistence** now happens at one point (the terminal `result`
+  event), gated on non-empty results — the empty-store conversation path
+  persists nothing, as before, and conversation writes concentrate ahead of the
+  step-5 ConversationStore extraction. The only residual difference from the old
+  code is an untested mid-generation-crash edge (old: a dangling user message;
+  now: nothing) — "don't persist a half-failed turn" is the more defensible
+  behaviour.
+- **Eval now measures the shipped pipeline.** Deliberate, spec-accepted changes:
+  eval telemetry granularity collapses from per-lever stages
+  (`rewrite`/`rerank`/`refusal_check`) to the engine's `retrieve`/`generate`;
+  the dead `rewriter_cost_usd` field is dropped (verified: zero readers); and
+  multi-query dedup shifts from first-seen to best-score-and-truncate (the
+  *shipped* `MultiQueryRetriever` semantics) — a retrieval-metric shift that is
+  correct-by-definition once eval measures production. And because the gate is
+  checked before the no-documents branch, an eval run with an **empty index and
+  no refusal handler** now returns the no-documents sentinel instead of
+  generating from empty context (the old eval path) — latent, since eval always
+  ingests before querying.
+- **Deferred, with signal:** `hybrid` needs a live BM25 corpus kept in sync with
+  ingestion/deletion (a genuinely new feature), and `multi_query`'s production
+  wiring is held so it lands deliberately; `build_retriever` raises a clear
+  error pointing here rather than silently falling back. When hybrid *is*
+  enabled, `BM25HybridRetriever` emits empty `metadata`/`doc_id` (its corpus is
+  `chunk_id -> text`), so citations degrade — acceptable while the lever is off
+  by default.
+- **New coverage:** contract tests across every adapter; engine tests with a
+  fake Retriever and fake LLM asserting sync and streaming issue identical
+  answer instructions; a factory strategy→type test; and the eval↔production
+  parity test.

docs/adr/0005-domain-value-types.md

@@ -0,0 +1,33 @@
+# ADR 0005 — Value types belong to no module
+
+- **Status:** Accepted
+- **Sequencing:** Follow-up to the 2026-09-09 architecture review; not part of issue #16's original eight steps.
+- **Date:** 2026-09-09
+
+## Context
+
+ADR 0004 cut a `Retriever` seam: one Protocol, `retrieve(query, top_k) -> list[SearchResult]`, with adapters that either conform or compose. The seam works. But `SearchResult` — the type the seam is defined *in terms of* — was declared in `src/vector_store.py`, the module whose first statement is `import chromadb`.
+
+The consequence: every module that names the seam imports the storage vendor. Ten of them did — the whole `src/retrieval/` package (`base`, `dense`, `hybrid`, `reranker`, `query_rewriter`, `refusal_handler`), the whole `src/query_engine/` package (`engine`, `prompt`, `streaming`), and `src/eval/pipeline_factory.py`. `src/retrieval/base.py`, which exists only to declare the Protocol, could not be read or imported without ChromaDB present.
+
+`Document` and `Chunk` had the same shape of problem one step earlier: naming a chunk meant importing the file-parsing module, so `tests/conftest.py` imported `src/document_loader.py` — with its `pypdf`, `python-docx` and `bs4` branches — to construct a fixture.
+
+A second, related problem: `ChromaVectorStore.query()` converts ChromaDB's distance to a similarity with `score = max(0.0, 1.0 - distance)`, which is only correct in cosine space. Nothing enforced that. Instead `metadata={"hnsw:space": "cosine"}` was spelled out at **nine construction sites** (production, the eval pipeline, and seven test fixtures). A site that omitted it got silently wrong similarity scores — no error, just worse answers.
+
+## Decision
+
+**A leaf module, `src/domain.py`,** owning `Document`, `Chunk`, `SearchResult`, and `content_hash`. It imports nothing from this package, so anything may import it without a cycle and without pulling in an implementation. The modules that previously *defined* these types now import them like everyone else.
+
+Plain dataclasses, not Pydantic: these cross internal seams where both sides are trusted. Validation stays at the API boundary, which has its own schemas.
+
+**The store owns its own invariant.** `ChromaVectorStore.open(client, name, embedding_function=...)` creates the collection with `SPACE_METADATA` and wraps it. All nine construction sites now go through it. The bare constructor remains for the case of an existing collection known to be cosine, and its docstring says so.
+
+A `collection` property replaces the two legitimate reads of `_collection` from outside (facade wiring, teardown naming).
+
+## Consequences
+
+- **The seam can be named without the vendor.** `import src.domain` pulls in neither `chromadb` nor `openai`; a test pins this by subprocess. The `retrieval` and `query_engine` *packages* still import the store through their `__init__` re-exports, which is correct — `DenseRetriever` genuinely wraps a store. The point is that the *type at the seam* no longer requires it.
+- **Ids are unchanged.** `content_hash` is the same full SHA-256 as the previous `_hash_text`; parity was verified against the old implementation before the move, including the non-ASCII path. These ids are persisted in SQLite and ChromaDB, so a change would have orphaned every stored chunk. An intermediate version of this change truncated the digest to 16 characters and was caught by that check — the truncation had been deliberately fixed earlier and is recorded in `content_hash`'s docstring so it is not reintroduced a third time.
+- **The cosine invariant has one owner.** Nine repetitions became one constant. `metadata={"hnsw:space": "cosine"}` no longer appears anywhere outside `src/vector_store.py`.
+- **Fixtures got lighter.** `tests/conftest.py` no longer imports the file-parsing module to build a `Chunk`.
+- **New coverage:** `tests/test_domain.py` pins id derivation (including that two documents containing the same paragraph keep distinct chunk ids), the full-digest length, and the vendor-free import; `tests/test_vector_store_chroma.py` pins that `open()` produces a cosine collection and honours an explicit embedding function.