From 3197b91e0a47ce3c396e0a7153dfc6431baa7b9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20Re=C3=A9?= Date: Sun, 12 Jul 2026 12:18:53 +0200 Subject: [PATCH] RAG hybrid retrieval: BM25 + knn with RRF, integration test, prompt fix (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-MVP step P1 (#460), pulled forward: hybrid retrieval is the industry default because keyword and vector search have complementary failure modes (exact names/codes vs. paraphrase). - P1.1 hybrid retrieval in the query pipeline: parent-level Reciprocal Rank Fusion (k=60) of the knn chunk ranking (collapsed to parents) and the classic keyword (BM25) parent ranking; keyword-only parents get their leading chunks fetched for the prompt context. Hybrid is the default; KITCONCEPT_SOLR_RAG_RETRIEVAL=knn switches back to pure knn (for corpus evaluation). Client-side fusion is drop-in replaceable by Solr's native RRF when 9.11/10.1 ships. - The BM25 side is an exact copy of the @solr main query: same fields and boosts including searchwords^1000 (the editorial "pin a document for a keyword" mechanism) and the -showinsearch:False exclusion - both must behave identically in the AI search. Field notes and the refactoring criteria are documented in the module. - P1.2 CI-runnable integration test: real docker Solr with a deterministic mock LLM (topic-axis unit vectors), no network needed. - Prompt: context chunks are labeled with their document title instead of bracketed numbers - models reliably leaked "[1]"-style references into answers (complied with the no-references instruction only about half the time); with title labels the leak disappears. - Docs: Step P1 design decisions in IMPLEMENTATION-79.md; the facet/tab scoping question recorded as the remaining open point in SPECIFICATION-79.md §8 (to be discussed with Dante and Timo). Verified against the bundled German demo corpus: golden questions 19/19 retrieval hits, 2/2 declines, zero answers containing bracketed references; RAG test suite 128 passed. --- IMPLEMENTATION-79.md | 50 ++++- SPECIFICATION-79.md | 25 +++ backend/news/79.feature.3 | 1 + backend/src/kitconcept/solr/rag/config.py | 13 ++ backend/src/kitconcept/solr/rag/pipeline.py | 196 +++++++++++++++++--- backend/src/kitconcept/solr/rag/prompt.py | 15 +- backend/tests/rag/test_integration_stack.py | 159 ++++++++++++++++ backend/tests/rag/test_pipeline.py | 161 +++++++++++++++- backend/tests/rag/test_prompt.py | 20 +- news/79.feature | 1 + 10 files changed, 596 insertions(+), 45 deletions(-) create mode 100644 backend/news/79.feature.3 create mode 100644 backend/tests/rag/test_integration_stack.py diff --git a/IMPLEMENTATION-79.md b/IMPLEMENTATION-79.md index acea533b..6733c6a0 100644 --- a/IMPLEMENTATION-79.md +++ b/IMPLEMENTATION-79.md @@ -306,17 +306,62 @@ repository's estimates). **Step P1 — Hybrid RRF + tests (2d), first follow-up:** -- [ ] **P1.1 Hybrid retrieval (1d)**: BM25 (reusing the existing +- [x] **P1.1 Hybrid retrieval (1d)**: BM25 (reusing the existing `SolrSearch` query building) + knn as two Solr requests, client-side Reciprocal Rank Fusion (k=60) in `@rag-search`; designed to be swapped for Solr's native RRF combiner when 9.11/10.1 ships. Hybrid was originally MVP scope (empirical signal that pure vector is not enough on intranet content); the pure-knn MVP results should confirm the deferral was acceptable — if not, this item moves up. -- [ ] **P1.2 Test pass (1d)**: unit tests (chunker, LLM client mocked) + one +- [x] **P1.2 Test pass (1d)**: unit tests (chunker, LLM client mocked) + one integration test against the docker-compose stack with deterministic mock endpoints (CI needs no GPU/model). +### Implementation notes — Step P1 (design decisions made on the way) + +- **Fusion happens at the parent-document level.** Chunks are invisible + to keyword search by design (stored, not indexed), so BM25 ranks + parent documents, while the knn chunk hits are collapsed to their + parents. Client-side RRF (`k=60`) fuses the two rankings; ties + resolve toward the vector ranking. +- **The keyword scoring expression is an exact copy of the `@solr` + main query** — same fields, same boosts (decision after review): + - we DO want the same fields with the same weighting, including + `Subject`; + - we DO want `searchwords^1000` (the editorial keyword-pinning + mechanism) and the `-showinsearch:False` exclusion — both must + behave identically in the AI search (important); + - `id^0.75` is included; whether id matching is useful for natural + language questions may be revisited later (note); + - `text_prefix`/`text_suffix^0.75` are likely unneeded for full NL + questions (they serve terse/partial-word queries) but are included + for exact parity since their low boosts don't disturb the ranking; + may be revisited (note). + A pinning unit test spells out every clause, so the copy cannot + drift silently. NOT inherited (deliberately): facet/search-tab + conditions, highlighting, spellcheck, pagination — request-driven + UI machinery without meaning for the RAG query. +- **Shared query builder refactoring deferred** (overflow list): for + now the expression is copied and the production `@solr` service is + left untouched; the later refactoring extracts the common core with + the acceptance criteria: (a) new unit tests for the factored-out + parts, (b) the pre-existing solr service tests pass unchanged after + the refactoring, (c) the RAG route gets its own tests modeled on the + solr service test examples. +- **Keyword-only parents contribute context.** For a parent that only + the keyword ranking surfaced, the leading chunks are fetched from + Solr so its text reaches the model — otherwise a document found by + keyword search could appear as a source without being able to + influence the answer. +- **Retrieval mode override** `KITCONCEPT_SOLR_RAG_RETRIEVAL=knn` + (default `hybrid`) exists solely for the pure-vector vs. hybrid + comparison in the evaluation on the real corpus; not a supported + setting. +- The unit-test part of P1.2 had already been delivered inline with + Steps 1 and 3; the new piece is the integration test with a + deterministic mock LLM (topic-axis unit vectors, canned answer) + against the real docker Solr — CI-runnable with no network/model. + **AI presentation via tabs configuration** (decided with the team 2026-07-23, replaces the earlier "AI search" toggle; to be elaborated and implemented after the UX has been seen live in kitconcept.intranet): @@ -343,6 +388,7 @@ remains): | Full configuration surface: registry records for model names, topK, chunk size, prompt override | 0.5d | | Acceptance test flow + full CI wiring (after the search-UI integration, so acceptance tests target the real UI) | 1d | | Separate Solr core/ports for tests vs. local dev site (see known issue 1) | 0.5d | +| Extract a shared keyword-query builder used by both `@solr` and the RAG pipeline (criteria: unit tests for the shared part, old solr tests green, RAG route tests modeled on the solr test examples) | 0.5d | **Later roadmap** (tracked, not scheduled): full evaluation harness (Recall@k/MRR/nDCG as CI regression gate, RAGAS faithfulness/relevancy with diff --git a/SPECIFICATION-79.md b/SPECIFICATION-79.md index ea12815d..69423e70 100644 --- a/SPECIFICATION-79.md +++ b/SPECIFICATION-79.md @@ -331,6 +331,31 @@ Resolved open point: yet be produced manually, e2e tests follow when the editor settles. +Remaining open point: + +1. **Facet/tab conditions and the AI search** — requirements question, + **to be discussed with Dante and Timo in the review**. The classic + search presents tabbed results per content type with facet conditions + in selected tabs (e.g. the person search). Two distinct readings for + the AI search: + - *Facets as navigation over the results:* does **not** transfer — the + source list is a small top-K evidence set justifying the answer, not + an exhaustive listing to narrow down; filtering the evidence away + would undermine the fact-checking contract. + - *Facets/tabs as scope constraints on the question* (ask within a + tab or facet selection, e.g. a department in the person search): + transfers well and the architecture already supports it — the knn + query composes any filter query as a pre-filter (as security, path + and language do today). Adding tab/facet parameters to + `@rag-search` is a bounded, additive change reusing the classic + search's condition builders. Caveat: chunks do not denormalize + `portal_type` or the facet fields, so scoped retrieval needs either + those fields on the chunks (schema addition + reindex) or filtering + at the parent-collapse step — a real design decision. + Proposed MVP stance: whole-intranet scope (matches the single search + box); scoped RAG as a follow-up once the modal UX (external search + modal project) defines what scoping looks like. + ## 9. References Research summaries behind the recommendations (full reports in the planning diff --git a/backend/news/79.feature.3 b/backend/news/79.feature.3 new file mode 100644 index 00000000..e26f9f0e --- /dev/null +++ b/backend/news/79.feature.3 @@ -0,0 +1 @@ +Hybrid retrieval for the RAG search: parent-level Reciprocal Rank Fusion of the knn chunk ranking with the classic keyword query (an exact copy of the @solr main query incl. searchwords boost and showinsearch exclusion); prompt context labeled by document title instead of leaking bracketed reference numbers. @reebalazs diff --git a/backend/src/kitconcept/solr/rag/config.py b/backend/src/kitconcept/solr/rag/config.py index b01b2a19..a50cee07 100644 --- a/backend/src/kitconcept/solr/rag/config.py +++ b/backend/src/kitconcept/solr/rag/config.py @@ -53,6 +53,16 @@ # generation prompt). TOP_K = 5 +# Reciprocal Rank Fusion constant for hybrid retrieval (the standard +# value from Cormack et al. 2009; Solr's native RRF uses it as well). +RRF_K = 60 + +# Retrieval mode: "hybrid" (BM25 + vector, RRF-fused; the default) or +# "knn" (pure vector). The env override exists for the evaluation on +# the real corpus (compare the two modes); not a supported setting. +RETRIEVAL_HYBRID = "hybrid" +RETRIEVAL_KNN = "knn" + REGISTRY_ENABLED_KEY = "kitconcept.solr.rag_enabled" # Endpoint paths, relative to the server root URL. The defaults match @@ -70,6 +80,7 @@ ENV_CHAT_MODEL = "KITCONCEPT_SOLR_LLM_CHAT_MODEL" ENV_EMBED_PATH = "KITCONCEPT_SOLR_LLM_EMBED_PATH" ENV_CHAT_PATH = "KITCONCEPT_SOLR_LLM_CHAT_PATH" +ENV_RETRIEVAL = "KITCONCEPT_SOLR_RAG_RETRIEVAL" @dataclass(frozen=True) @@ -84,6 +95,7 @@ class RagConfig: chat_path: str = DEFAULT_CHAT_PATH embed_timeout: float = EMBED_TIMEOUT chat_timeout: float = CHAT_TIMEOUT + retrieval: str = RETRIEVAL_HYBRID def rag_enabled() -> bool: @@ -119,6 +131,7 @@ def get_rag_config() -> RagConfig | None: chat_model=os.environ.get(ENV_CHAT_MODEL, "").strip() or DEFAULT_CHAT_MODEL, embed_path=os.environ.get(ENV_EMBED_PATH, "").strip() or DEFAULT_EMBED_PATH, chat_path=os.environ.get(ENV_CHAT_PATH, "").strip() or DEFAULT_CHAT_PATH, + retrieval=os.environ.get(ENV_RETRIEVAL, "").strip() or RETRIEVAL_HYBRID, ) diff --git a/backend/src/kitconcept/solr/rag/pipeline.py b/backend/src/kitconcept/solr/rag/pipeline.py index 7416cfe3..70458e4c 100644 --- a/backend/src/kitconcept/solr/rag/pipeline.py +++ b/backend/src/kitconcept/solr/rag/pipeline.py @@ -1,15 +1,28 @@ """The RAG query pipeline: question -> retrieved chunks -> answer. -Implements the single-turn RAG search (SPECIFICATION-79.md §4): +Implements the single-turn RAG search (SPECIFICATION-79.md §4) with +hybrid retrieval: 1. embed the user's question (``search_query:`` prefix), 2. retrieve the top chunks via a ``{!knn}`` query — the existing security/path/language filter queries compose with the vector query as HNSW pre-filters, so permission trimming works unchanged, -3. collapse the chunk hits to their parent documents (the sources), -4. generate the answer with the general-purpose model, prompted with - the matched chunk texts (chunk-level context, decision 9) and - constrained to the provided context. +3. retrieve the top parent documents via the classic keyword (BM25) + query and fuse both rankings with client-side Reciprocal Rank + Fusion — hybrid is the industry default because keyword and vector + search have complementary failure modes (exact names/codes vs. + paraphrase). Solr's native RRF lands in 9.11/10.1; the client-side + fusion is drop-in replaceable by it, +4. assemble the context chunks for the fused parent ranking (chunks + from the knn hits; fetched from Solr for keyword-only parents), +5. generate the answer with the general-purpose model, prompted with + the chunk texts (chunk-level context, decision 9) and constrained + to the provided context. + +Fusion happens at the *parent document* level: chunks are invisible to +keyword search by design (their text is stored but not indexed), so +BM25 ranks parents, while the chunk hits of the knn side are collapsed +to their parents (parent-document retrieval). The pipeline is independent of the REST service so it can be tested with a faked Solr connection and LLM client, and reused (e.g. by a @@ -24,10 +37,14 @@ from kitconcept.solr.rag.client import LLMClient from kitconcept.solr.rag.client import LLMClientError from kitconcept.solr.rag.config import RagConfig +from kitconcept.solr.rag.config import RETRIEVAL_HYBRID +from kitconcept.solr.rag.config import RRF_K from kitconcept.solr.rag.config import TOP_K from kitconcept.solr.rag.prompt import build_prompt from kitconcept.solr.rag.prompt import strip_thinking from kitconcept.solr.rag.prompt import SYSTEM_PROMPT +from kitconcept.solr.services.solr_utils import escape +from kitconcept.solr.services.solr_utils import replace_reserved from plone import api from zope.component import queryUtility @@ -90,18 +107,28 @@ def run_rag_search( ) try: chunks = search_chunks(conn, vector, security_filter, path_prefix, lang) - sources = collapse_sources(conn, chunks) if chunks else [] + knn_parents = parent_ranking(chunks) + if config.retrieval == RETRIEVAL_HYBRID: + keyword_parents = search_keyword( + conn, question, security_filter, path_prefix, lang + ) + fused_parents = rrf_fuse([knn_parents, keyword_parents]) + else: + fused_parents = knn_parents + fused_parents = fused_parents[:TOP_K] + if not fused_parents: + # No matching (visible) content: not an error - the answer is + # that there is no answer. + return RagResult() + context_chunks = assemble_context(conn, chunks, fused_parents) + sources = build_sources(conn, fused_parents, context_chunks) except (SolrConnectionException, OSError) as e: # collective.solr raises raw socket errors (e.g. # ConnectionRefusedError) when the Solr server is down logger.warning("rag-search: Solr unavailable: %s", e) return RagResult.failure(ERROR_SOLR_UNAVAILABLE, str(e)) - if not chunks: - # No matching (visible) content: not an error - the answer is - # that there is no answer. - return RagResult() - prompt = build_prompt(question, chunks) + prompt = build_prompt(question, context_chunks) try: answer = client.chat(prompt, system=SYSTEM_PROMPT) except LLMClientError as e: @@ -148,35 +175,150 @@ def search_chunks( response.close() -def collapse_sources(conn, chunks: list[dict]) -> list[dict]: - """Parent documents of the matched chunks, in rank order. +def search_keyword( + conn, + question: str, + security_filter: str, + path_prefix: str | None = None, + lang: str | None = None, +) -> list[str]: + """Top-K parent documents for the classic keyword (BM25) query. + + The scoring expression is an exact copy of the ``@solr`` main + query (``SolrSearch._base_query``): same fields, same boosts — + including ``searchwords^1000`` (the editorial "pin a document for + a keyword" mechanism) and the ``-showinsearch:False`` exclusion, + both of which must behave identically in the AI search. Notes: - Parent-document retrieval: retrieval matches chunks, but the user - sees the parent documents as the sources. Parent metadata is - fetched from Solr in one query and merged with a snippet from the - best-ranked chunk of each parent. + - ``id^0.75``: kept for parity; whether id matching makes sense + for natural language questions may be revisited. + - ``text_prefix``/``text_suffix^0.75``: likely unneeded for full + NL questions (they serve terse/partial-word queries), but + included for exact parity since their low boosts don't disturb + the ranking; may be revisited. + + Not inherited (deliberately): facet/search-tab conditions, + highlighting, spellcheck, pagination — request-driven UI machinery + of the classic search page that has no meaning here and does not + affect the ranking. Extracting a shared query builder so the copy + cannot drift is a planned refactoring (see the overflow list). + + Chunks are excluded — they carry no indexed text anyway. """ + term = f"({escape(replace_reserved(question))})" + query = ( + f"+(Title:{term}^5 OR Description:{term}^2 OR id:{term}^0.75 " + f"OR text_prefix:{term}^0.75 OR text_suffix:{term}^0.75 " + f"OR default:{term} OR body_text:{term} OR SearchableText:{term} " + f"OR Subject:{term} OR searchwords:({term})^1000) -showinsearch:False" + ) + filter_queries = [security_filter, "-is_rag_chunk:true"] + if path_prefix: + portal_path = "/".join(api.portal.get().getPhysicalPath()) + prefix = portal_path + path_prefix.rstrip("/") + filter_queries.append(f'path_parents:"{prefix}"') + if lang: + filter_queries.append(f"Language:({lang} OR any)") + response = conn.search( + q=query, + fq=filter_queries, + fl="UID", + rows=TOP_K, + ) + try: + results = SolrResponse(response).results() + finally: + response.close() + return [flare["UID"] for flare in results] + + +def parent_ranking(chunks: list[dict]) -> list[str]: + """Parent UIDs of the chunk hits, deduplicated, in rank order.""" order: list[str] = [] - best_chunk: dict[str, dict] = {} + seen = set() for chunk in chunks: parent_uid = chunk.get("parent_uid") - if not parent_uid: - continue - if parent_uid not in best_chunk: + if parent_uid and parent_uid not in seen: + seen.add(parent_uid) order.append(parent_uid) - best_chunk[parent_uid] = chunk - if not order: - return [] - parents = fetch_parents(conn, order) + return order + + +def rrf_fuse(rankings: list[list[str]], k: int = RRF_K) -> list[str]: + """Reciprocal Rank Fusion of ranked UID lists. + + ``score(d) = sum over rankings of 1 / (k + rank(d))`` — the + standard fusion that needs no score normalization (Cormack et al. + 2009). Ties keep the order of the first ranking. + """ + scores: dict[str, float] = {} + for ranking in rankings: + for index, uid in enumerate(ranking): + scores[uid] = scores.get(uid, 0.0) + 1.0 / (k + index + 1) + return sorted(scores, key=lambda uid: -scores[uid]) + + +def assemble_context( + conn, knn_chunks: list[dict], fused_parents: list[str] +) -> list[dict]: + """Context chunks for the fused parent ranking, capped at TOP_K. + + Chunks retrieved by the knn query are used as-is; for parents that + only the keyword ranking surfaced, the leading chunks are fetched + from Solr — their text must reach the model, otherwise a document + found by keyword search could not contribute to the answer. + """ + by_parent: dict[str, list[dict]] = {} + for chunk in knn_chunks: + by_parent.setdefault(chunk.get("parent_uid"), []).append(chunk) + context: list[dict] = [] + for parent_uid in fused_parents: + if parent_uid in by_parent: + context.extend(by_parent[parent_uid]) + else: + context.extend(fetch_leading_chunks(conn, parent_uid)) + if len(context) >= TOP_K: + break + return context[:TOP_K] + + +def fetch_leading_chunks(conn, parent_uid: str, limit: int = 2) -> list[dict]: + """The first chunks of a document (for keyword-only parents).""" + response = conn.search( + q=f'+parent_uid:"{parent_uid}" +is_rag_chunk:true', + sort="chunk_index asc", + fl=CHUNK_FIELD_LIST, + rows=limit, + ) + try: + return list(SolrResponse(response).results()) + finally: + response.close() + + +def build_sources( + conn, fused_parents: list[str], context_chunks: list[dict] +) -> list[dict]: + """Source documents in fused rank order. + + Parent-document retrieval: the user sees the parent documents as + the sources. Parent metadata is fetched from Solr in one query and + merged with a snippet from the best-ranked context chunk of each + parent (empty when a parent contributed no context). + """ + best_chunk: dict[str, dict] = {} + for chunk in context_chunks: + best_chunk.setdefault(chunk.get("parent_uid"), chunk) + parents = fetch_parents(conn, fused_parents) portal = api.portal.get() portal_path = "/".join(portal.getPhysicalPath()) portal_url = portal.absolute_url() sources = [] - for parent_uid in order: + for parent_uid in fused_parents: parent = parents.get(parent_uid, {}) - chunk = best_chunk[parent_uid] + chunk = best_chunk.get(parent_uid, {}) path_string = parent.get("path_string") or chunk.get("path_string", "") url = ( portal_url + path_string[len(portal_path) :] diff --git a/backend/src/kitconcept/solr/rag/prompt.py b/backend/src/kitconcept/solr/rag/prompt.py index 7ae737ec..12d70816 100644 --- a/backend/src/kitconcept/solr/rag/prompt.py +++ b/backend/src/kitconcept/solr/rag/prompt.py @@ -19,7 +19,8 @@ " find the answer in the documentation - never invent information." " Answer in the language of the question. Be concise: one or two" " short paragraphs, no headings and no lists unless the question" - " asks for an enumeration." + " asks for an enumeration. When you refer to a document, use its" + " title." ) PROMPT_TEMPLATE = ( @@ -33,12 +34,18 @@ def build_prompt(question: str, chunks: list[dict]) -> str: - """One prompt containing the question and the retrieved context.""" + """One prompt containing the question and the retrieved context. + + Context chunks are labeled with their document title, NOT numbered: + models reliably leak "[1]"-style context numbers into the answer + (instructions against it proved insufficient), and the numbering + is invisible to the user. Titles are safe to reference. + """ parts = [] - for index, chunk in enumerate(chunks[:TOP_K], start=1): + for chunk in chunks[:TOP_K]: title = chunk.get("parent_title", "") text = chunk.get("chunk_text", "") - parts.append(f"[{index}] {title}\n{text}") + parts.append(f'Document "{title}":\n{text}') return PROMPT_TEMPLATE.format(context="\n\n".join(parts), question=question) diff --git a/backend/tests/rag/test_integration_stack.py b/backend/tests/rag/test_integration_stack.py new file mode 100644 index 00000000..253cc3af --- /dev/null +++ b/backend/tests/rag/test_integration_stack.py @@ -0,0 +1,159 @@ +"""Integration test of the full RAG path with a deterministic mock LLM. + +Runs in CI: needs the docker Solr (like the service tests) but no LLM +server — the client methods are patched with deterministic fakes, so +this covers everything except the LLM itself: chunk indexing through +the real Solr XML update path, the schema fields, the {!knn} and +keyword queries, hybrid fusion, and the @rag-search service over HTTP. + +The fake embeddings map topics to orthogonal unit vectors, so vector +retrieval ranks deterministically. +""" + +from kitconcept.solr.rag.client import LLMClient +from kitconcept.solr.rag.config import ENV_URL +from plone import api +from plone.app.testing import SITE_OWNER_NAME +from plone.app.testing import SITE_OWNER_PASSWORD +from plone.restapi.testing import RelativeSession +from unittest import mock +from zope.component.hooks import setSite + +import pytest +import transaction + + +DIMENSION = 768 + +TOPIC_AXES = { + "vacation": 0, + "security": 1, +} +OTHER_AXIS = 2 + +CANNED_ANSWER = "According to the vacation policy you get 30 days." + + +def topic_vector(text: str) -> list[float]: + """A unit vector on the axis of the text's topic.""" + lowered = text.lower() + axis = OTHER_AXIS + for topic, topic_axis in TOPIC_AXES.items(): + if topic in lowered: + axis = topic_axis + break + vector = [0.0] * DIMENSION + vector[axis] = 1.0 + return vector + + +def fake_embed_documents(self, texts): + return [topic_vector(text) for text in texts] + + +def fake_embed_query(self, text): + return topic_vector(text) + + +def fake_chat(self, prompt, system=None): + return f"reasoning{CANNED_ANSWER}" + + +@pytest.fixture() +def mock_llm(monkeypatch): + """Deterministic LLM client + enabled feature, no network.""" + monkeypatch.setenv(ENV_URL, "http://mock-llm.invalid") + with ( + mock.patch.object(LLMClient, "embed_documents", fake_embed_documents), + mock.patch.object(LLMClient, "embed_query", fake_embed_query), + mock.patch.object(LLMClient, "chat", fake_chat), + ): + yield + + +@pytest.fixture() +def portal(functional, solr_service, mock_llm): + portal = functional["app"]["plone"] + setSite(portal) + with api.env.adopt_roles(["Manager", "Member"]): + api.portal.set_registry_record("collective.solr.active", True) + api.portal.set_registry_record("kitconcept.solr.rag_enabled", True) + maintenance = api.content.get_view( + "solr-maintenance", portal, functional["request"] + ) + maintenance.clear() + for doc_id, title, body in [ + ( + "vacation-policy", + "Vacation policy", + "Employees get 30 days of vacation per year.", + ), + ( + "it-rules", + "IT rules", + "Security guidelines: two-factor authentication required.", + ), + ]: + doc = api.content.create( + container=portal, type="Document", id=doc_id, title=title + ) + doc.blocks = {"b1": {"@type": "slate", "plaintext": body}} + doc.blocks_layout = {"items": ["b1"]} + doc.reindexObject() + transaction.commit() + yield portal + with api.env.adopt_roles(["Manager"]): + for doc_id in ["vacation-policy", "it-rules"]: + if doc_id in portal: + api.content.delete(portal[doc_id]) + api.portal.set_registry_record("kitconcept.solr.rag_enabled", False) + api.portal.set_registry_record("collective.solr.active", False) + transaction.commit() + + +@pytest.fixture() +def manager_session(portal): + session = RelativeSession(portal.absolute_url()) + session.headers.update({"Accept": "application/json"}) + session.auth = (SITE_OWNER_NAME, SITE_OWNER_PASSWORD) + return session + + +class TestRagIntegrationWithMockLLM: + def test_full_path_vacation_question(self, manager_session): + response = manager_session.get( + "/@rag-search", + params={"q": "How much vacation do I get?"}, + ) + assert response.status_code == 200 + data = response.json() + assert data["error"] is None + # canned answer passed through, thinking block stripped + assert data["answer"] == CANNED_ANSWER + # vector retrieval ranks the on-topic document first + assert data["sources"] + assert data["sources"][0]["title"] == "Vacation policy" + # hybrid: the keyword hit ("vacation" in title/text) is fused in + titles = [source["title"] for source in data["sources"]] + assert "Vacation policy" in titles + + def test_full_path_security_question(self, manager_session): + response = manager_session.get( + "/@rag-search", + params={"q": "What are the security rules?"}, + ) + data = response.json() + assert data["answer"] == CANNED_ANSWER + assert data["sources"][0]["title"] == "IT rules" + assert data["sources"][0]["snippet"] + + def test_chunks_indexed_with_fake_vectors(self, portal, solr_service): + import requests + + select = solr_service.split("/admin/")[0] + "/select" + response = requests.post( + select, + data={"q": "is_rag_chunk:true", "rows": 0, "wt": "json"}, + timeout=10, + ) + assert response.json()["response"]["numFound"] >= 2 diff --git a/backend/tests/rag/test_pipeline.py b/backend/tests/rag/test_pipeline.py index 920e296b..0cdc2c57 100644 --- a/backend/tests/rag/test_pipeline.py +++ b/backend/tests/rag/test_pipeline.py @@ -1,13 +1,18 @@ from kitconcept.solr.rag import pipeline as pipeline_module from kitconcept.solr.rag.client import LLMClientError from kitconcept.solr.rag.config import RagConfig -from kitconcept.solr.rag.pipeline import collapse_sources +from kitconcept.solr.rag.config import RETRIEVAL_KNN +from kitconcept.solr.rag.pipeline import assemble_context +from kitconcept.solr.rag.pipeline import build_sources from kitconcept.solr.rag.pipeline import ERROR_EMBEDDING_FAILED from kitconcept.solr.rag.pipeline import ERROR_GENERATION_FAILED from kitconcept.solr.rag.pipeline import ERROR_SOLR_UNAVAILABLE from kitconcept.solr.rag.pipeline import format_vector +from kitconcept.solr.rag.pipeline import parent_ranking +from kitconcept.solr.rag.pipeline import rrf_fuse from kitconcept.solr.rag.pipeline import run_rag_search from kitconcept.solr.rag.pipeline import search_chunks +from kitconcept.solr.rag.pipeline import search_keyword from unittest import mock import pytest @@ -90,11 +95,15 @@ def environment(conn): mock.patch.object( pipeline_module, "search_chunks", return_value=list(CHUNKS) ) as searcher, + mock.patch.object( + pipeline_module, "search_keyword", return_value=[] + ) as keyword, + mock.patch.object(pipeline_module, "fetch_leading_chunks", return_value=[]), mock.patch.object(pipeline_module, "fetch_parents", return_value=dict(PARENTS)), ): llm_class.return_value.embed_query.return_value = [0.1, 0.2] llm_class.return_value.chat.return_value = "The answer is 30 days." - yield {"llm": llm_class, "searcher": searcher} + yield {"llm": llm_class, "searcher": searcher, "keyword": keyword} class TestRunRagSearch: @@ -150,19 +159,19 @@ def test_no_chunks_found_is_not_an_error(self, environment): assert result.sources == [] -class TestCollapseSources: - def test_parents_in_rank_order_deduplicated(self, conn): +class TestBuildSources: + def test_parents_in_given_order(self, conn): with mock.patch.object( pipeline_module, "fetch_parents", return_value=dict(PARENTS) ): - sources = collapse_sources(conn, CHUNKS) + sources = build_sources(conn, ["uid-a", "uid-b"], CHUNKS) assert [s["UID"] for s in sources] == ["uid-a", "uid-b"] def test_source_shape(self, conn): with mock.patch.object( pipeline_module, "fetch_parents", return_value=dict(PARENTS) ): - sources = collapse_sources(conn, CHUNKS) + sources = build_sources(conn, ["uid-a", "uid-b"], CHUNKS) source = sources[0] assert source["@id"] == "http://nohost/plone/vacation-policy" assert source["title"] == "Vacation policy" @@ -173,10 +182,18 @@ def test_source_shape(self, conn): def test_missing_parent_metadata_falls_back_to_chunk(self, conn): with mock.patch.object(pipeline_module, "fetch_parents", return_value={}): - sources = collapse_sources(conn, CHUNKS) + sources = build_sources(conn, ["uid-a", "uid-b"], CHUNKS) assert sources[0]["title"] == "Vacation policy" assert sources[0]["@id"] == "http://nohost/plone/vacation-policy" + def test_parent_without_context_chunk_has_empty_snippet(self, conn): + with mock.patch.object( + pipeline_module, "fetch_parents", return_value=dict(PARENTS) + ): + sources = build_sources(conn, ["uid-a", "uid-b"], CHUNKS[:1]) + assert sources[1]["snippet"] == "" + assert sources[1]["title"] == "Cafeteria" + class TestSearchChunks: def fake_response(self): @@ -208,3 +225,133 @@ def test_path_prefix_filter(self, conn): def test_language_filter(self, conn): params = self.search_params(conn, lang="en") assert "Language:(en OR any)" in params["fq"] + + +class TestHybridRetrieval: + def test_hybrid_fuses_keyword_ranking(self, environment): + # keyword search promotes uid-b ahead of knn's uid-a ordering + environment["keyword"].return_value = ["uid-b", "uid-a"] + result = run_rag_search("q", CONFIG, SECURITY_FQ) + # both rank lists: knn [uid-a, uid-b], keyword [uid-b, uid-a] + # -> RRF ties resolved by first (knn) ranking order + assert [s["UID"] for s in result.sources] == ["uid-a", "uid-b"] + environment["keyword"].assert_called_once() + + def test_keyword_only_parent_joins_sources(self, environment): + environment["keyword"].return_value = ["uid-a", "uid-c"] + parents = dict(PARENTS) + parents["uid-c"] = { + "UID": "uid-c", + "Title": "Third doc", + "Description": "", + "Type": "Page", + "path_string": "/plone/third", + } + with mock.patch.object(pipeline_module, "fetch_parents", return_value=parents): + result = run_rag_search("q", CONFIG, SECURITY_FQ) + assert "uid-c" in [s["UID"] for s in result.sources] + + def test_knn_mode_skips_keyword_search(self, environment): + config = RagConfig( + base_url=CONFIG.base_url, + token=None, + embed_model=CONFIG.embed_model, + chat_model=CONFIG.chat_model, + retrieval=RETRIEVAL_KNN, + ) + result = run_rag_search("q", config, SECURITY_FQ) + environment["keyword"].assert_not_called() + assert result.error is None + + +class TestRrfFuse: + def test_agreement_wins(self): + fused = rrf_fuse([["a", "b", "c"], ["b", "a", "c"]]) + # a: 1/61 + 1/62; b: 1/62 + 1/61 -> tie broken by first list + assert fused[0] == "a" + assert set(fused) == {"a", "b", "c"} + + def test_item_in_both_lists_beats_single_list_items(self): + fused = rrf_fuse([["a", "b"], ["c", "b"]]) + assert fused[0] == "b" + + def test_single_ranking_passthrough(self): + assert rrf_fuse([["x", "y", "z"]]) == ["x", "y", "z"] + + def test_empty(self): + assert rrf_fuse([[], []]) == [] + + +class TestParentRanking: + def test_dedupes_preserving_order(self): + assert parent_ranking(CHUNKS) == ["uid-a", "uid-b"] + + def test_skips_chunks_without_parent(self): + assert parent_ranking([{"chunk_text": "x"}]) == [] + + +class TestAssembleContext: + def test_knn_chunks_used_in_fused_order(self, conn): + context = assemble_context(conn, CHUNKS, ["uid-b", "uid-a"]) + assert [c["UID"] for c in context] == [ + "uid-b#rag-0", + "uid-a#rag-1", + "uid-a#rag-0", + ] + + def test_keyword_only_parent_chunks_are_fetched(self, conn): + fetched = [ + { + "UID": "uid-c#rag-0", + "parent_uid": "uid-c", + "parent_title": "Third doc", + "chunk_text": "Third doc text.", + } + ] + with mock.patch.object( + pipeline_module, "fetch_leading_chunks", return_value=fetched + ) as fetcher: + context = assemble_context(conn, CHUNKS, ["uid-c", "uid-a"]) + fetcher.assert_called_once_with(conn, "uid-c") + assert context[0]["UID"] == "uid-c#rag-0" + assert context[1]["UID"] == "uid-a#rag-1" + + +class TestSearchKeyword: + def fake_search(self, conn, **kwargs): + conn.search.return_value = mock.Mock() + with mock.patch.object(pipeline_module, "SolrResponse") as solr_response: + solr_response.return_value.results.return_value = [] + search_keyword(conn, "usb stick?", SECURITY_FQ, **kwargs) + return conn.search.call_args.kwargs + + def test_query_is_exact_copy_of_solr_main_query(self, conn): + """Pin the scoring expression to the @solr main query. + + This test intentionally spells out every clause and boost of + SolrSearch._base_query: if either side changes, it must fail, + so the copies cannot drift apart silently. A shared query + builder replacing the copy is a planned refactoring. + """ + params = self.fake_search(conn) + term = "(usb stick\\?)" + assert params["q"] == ( + f"+(Title:{term}^5 OR Description:{term}^2 OR id:{term}^0.75 " + f"OR text_prefix:{term}^0.75 OR text_suffix:{term}^0.75 " + f"OR default:{term} OR body_text:{term} OR SearchableText:{term} " + f"OR Subject:{term} OR searchwords:({term})^1000) " + "-showinsearch:False" + ) + + def test_filters(self, conn): + params = self.fake_search(conn) + assert SECURITY_FQ in params["fq"] + assert "-is_rag_chunk:true" in params["fq"] + + def test_question_is_escaped(self, conn): + conn.search.return_value = mock.Mock() + with mock.patch.object(pipeline_module, "SolrResponse") as solr_response: + solr_response.return_value.results.return_value = [] + search_keyword(conn, 'evil" OR *:*', SECURITY_FQ) + query = conn.search.call_args.kwargs["q"] + assert '"' not in query.split("Title:")[1].split("^")[0].replace('\\"', "") diff --git a/backend/tests/rag/test_prompt.py b/backend/tests/rag/test_prompt.py index 0843f77e..b6b1c53e 100644 --- a/backend/tests/rag/test_prompt.py +++ b/backend/tests/rag/test_prompt.py @@ -15,26 +15,36 @@ def test_contains_question_and_context(self): [chunk("Vacation policy", "30 days of paid vacation.")], ) assert "Question: How many vacation days?" in prompt - assert "[1] Vacation policy\n30 days of paid vacation." in prompt + assert 'Document "Vacation policy":\n30 days of paid vacation.' in prompt assert "based only on the context documents" in prompt - def test_chunks_are_numbered_in_order(self): + def test_chunks_are_labeled_by_title_in_order(self): prompt = build_prompt( "q", [chunk("First", "text one"), chunk("Second", "text two")], ) - assert prompt.index("[1] First") < prompt.index("[2] Second") + assert prompt.index('Document "First":') < prompt.index('Document "Second":') def test_context_is_capped_at_top_k(self): chunks = [chunk(f"Doc {i}", f"text {i}") for i in range(TOP_K + 3)] prompt = build_prompt("q", chunks) - assert f"[{TOP_K}] " in prompt - assert f"[{TOP_K + 1}] " not in prompt + assert prompt.count('Document "') == TOP_K + + def test_no_bracketed_numbering_in_context(self): + # Models reliably leak "[1]"-style context numbers into user + # facing answers, so the prompt must not contain any: chunks + # are labeled by document title instead. + chunks = [chunk(f"Doc {i}", f"text {i}") for i in range(3)] + prompt = build_prompt("q", chunks) + assert "[1]" not in prompt + assert "[2]" not in prompt def test_system_prompt_constrains_generation(self): assert "based only on the provided context" in SYSTEM_PROMPT assert "could not find the answer" in SYSTEM_PROMPT assert "language of the question" in SYSTEM_PROMPT + # documents are referenced by title in the answer + assert "use its title" in SYSTEM_PROMPT class TestStripThinking: diff --git a/news/79.feature b/news/79.feature index 8eb0070e..775dbb9a 100644 --- a/news/79.feature +++ b/news/79.feature @@ -1,2 +1,3 @@ Add the foundations of the RAG AI search: LLM client (embeddings + chat), the AI search toggle with LLM endpoint configuration, structure-aware chunking, index-time chunk embedding into Solr, and full-reindex support. The feature is off unless configured. @reebalazs Add the @rag-search endpoint: single-turn RAG search returning a generated answer with the source documents, permission-trimmed vector retrieval, structured errors. @reebalazs +Hybrid retrieval for the RAG search: keyword (BM25) and vector rankings fused with Reciprocal Rank Fusion. @reebalazs