From eb21206d3c3fd8ae97ca60380a0f1af219955fee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20Re=C3=A9?= Date: Tue, 8 Sep 2026 11:28:05 +0200 Subject: [PATCH] RAG: keep chunking-excluded types out of keyword retrieval and sources (#124) Image content is excluded from chunking, but the exclusion only existed on the indexing side: the BM25 keyword leg still ranked Images by title, so they were cited as sources with an empty snippet while the answer could not know anything about them (the exact pollution the exclusion was meant to prevent). - search_keyword: mirror EXCLUDED_PORTAL_TYPES in the filter queries. - build_sources: a parent without any context chunk contributed nothing to the answer and is no longer cited. - SolrExtraConditions.from_encoded: an empty string (a bare extra_conditions= URL parameter from the results page URL rewrite) means no conditions, instead of logging an invalid-base64 warning on every request. --- backend/news/124.bugfix | 1 + backend/src/kitconcept/solr/rag/pipeline.py | 17 ++++++- .../solr/services/solr_utils_extra.py | 5 +- backend/tests/rag/test_pipeline.py | 48 +++++++++++++++++-- .../utils/test_utils_extra_conditions.py | 16 +++++++ 5 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 backend/news/124.bugfix diff --git a/backend/news/124.bugfix b/backend/news/124.bugfix new file mode 100644 index 00000000..5ee3719d --- /dev/null +++ b/backend/news/124.bugfix @@ -0,0 +1 @@ +RAG: documents that contributed no context (e.g. Images, which are excluded from chunking) are no longer retrieved by the keyword leg nor cited as sources. @reebalazs diff --git a/backend/src/kitconcept/solr/rag/pipeline.py b/backend/src/kitconcept/solr/rag/pipeline.py index f30677e7..37933210 100644 --- a/backend/src/kitconcept/solr/rag/pipeline.py +++ b/backend/src/kitconcept/solr/rag/pipeline.py @@ -37,6 +37,7 @@ 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.processor import EXCLUDED_PORTAL_TYPES from kitconcept.solr.rag.config import RETRIEVAL_HYBRID from kitconcept.solr.rag.config import RRF_K from kitconcept.solr.rag.config import TOP_K @@ -226,6 +227,14 @@ def search_keyword( f"OR Subject:{term} OR searchwords:({term})^1000) -showinsearch:False" ) filter_queries = [security_filter, "-is_rag_chunk:true"] + # Mirror the chunking policy: types excluded from chunking (see + # EXCLUDED_PORTAL_TYPES) can never contribute context, so the + # keyword leg must not surface them either - otherwise they end up + # cited as sources with an empty snippet while the answer cannot + # know anything about them. + filter_queries.extend( + f'-portal_type:"{ptype}"' for ptype in sorted(EXCLUDED_PORTAL_TYPES) + ) if extra_filters: filter_queries.extend(extra_filters) if path_prefix: @@ -319,7 +328,9 @@ def build_sources( 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). + parent. A parent without any context chunk contributed nothing to + the answer and is not cited (e.g. a keyword match on a document + that has no chunks, or a parent past the context cap). """ best_chunk: dict[str, dict] = {} for chunk in context_chunks: @@ -332,8 +343,10 @@ def build_sources( sources = [] for parent_uid in fused_parents: + chunk = best_chunk.get(parent_uid) + if chunk is None: + continue parent = parents.get(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/services/solr_utils_extra.py b/backend/src/kitconcept/solr/services/solr_utils_extra.py index 930db14b..fb8e372d 100644 --- a/backend/src/kitconcept/solr/services/solr_utils_extra.py +++ b/backend/src/kitconcept/solr/services/solr_utils_extra.py @@ -28,7 +28,10 @@ def __init__(self, config: dict): @classmethod def from_encoded(cls, raw: str): - if raw is not None: + # An empty string (e.g. a bare extra_conditions= URL + # parameter from the results page) means no conditions, like an + # absent parameter - not invalid input worth a log warning. + if raw: try: config = json.loads(base64.b64decode(raw)) except ( diff --git a/backend/tests/rag/test_pipeline.py b/backend/tests/rag/test_pipeline.py index 3ae005af..6ae2f4a9 100644 --- a/backend/tests/rag/test_pipeline.py +++ b/backend/tests/rag/test_pipeline.py @@ -186,13 +186,16 @@ def test_missing_parent_metadata_falls_back_to_chunk(self, conn): 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): + def test_parent_without_context_chunk_is_not_cited(self, conn): + # A parent that contributed no context chunk must not appear + # as a source: the answer cannot know anything about it (the + # observable symptom was an Image cited with an empty snippet + # while the answer denied knowing the image). 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" + assert [s["UID"] for s in sources] == ["uid-a"] class TestSearchChunks: @@ -256,10 +259,41 @@ def test_keyword_only_parent_joins_sources(self, environment): "Type": "Page", "path_string": "/plone/third", } - with mock.patch.object(pipeline_module, "fetch_parents", return_value=parents): + chunk_c = { + "UID": "uid-c#rag-0", + "parent_uid": "uid-c", + "parent_title": "Third doc", + "chunk_text": "Third doc leading text.", + "path_string": "/plone/third", + } + with ( + mock.patch.object(pipeline_module, "fetch_parents", return_value=parents), + mock.patch.object( + pipeline_module, "fetch_leading_chunks", return_value=[chunk_c] + ), + ): result = run_rag_search("q", CONFIG, SECURITY_FQ) assert "uid-c" in [s["UID"] for s in result.sources] + def test_keyword_only_parent_without_chunks_is_not_cited(self, environment): + # A keyword match on a document that has no chunks (e.g. an + # Image indexed before the type was excluded, or a document + # whose embedding failed) contributes nothing to the answer, + # so it must not be cited. fetch_leading_chunks returns [] via + # the environment fixture. + environment["keyword"].return_value = ["uid-a", "uid-c"] + parents = dict(PARENTS) + parents["uid-c"] = { + "UID": "uid-c", + "Title": "a green cat", + "Description": "", + "Type": "Image", + "path_string": "/plone/green-cat.jpg", + } + with mock.patch.object(pipeline_module, "fetch_parents", return_value=parents): + result = run_rag_search("q", CONFIG, SECURITY_FQ) + assert "uid-c" not in [s["UID"] for s in result.sources] + def test_extra_filters_reach_both_legs(self, environment): run_rag_search( "q", @@ -369,6 +403,12 @@ def test_filters(self, conn): assert SECURITY_FQ in params["fq"] assert "-is_rag_chunk:true" in params["fq"] + def test_chunking_excluded_types_are_filtered(self, conn): + # Types excluded from chunking (e.g. Image) can never + # contribute context, so the keyword leg must not rank them. + params = self.fake_search(conn) + assert '-portal_type:"Image"' 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: diff --git a/backend/tests/utils/test_utils_extra_conditions.py b/backend/tests/utils/test_utils_extra_conditions.py index e68ec5a6..e3eeba9f 100644 --- a/backend/tests/utils/test_utils_extra_conditions.py +++ b/backend/tests/utils/test_utils_extra_conditions.py @@ -174,3 +174,19 @@ def test_query_list_string_in_invalid_term_type(self): obj = SolrExtraConditions(config) with pytest.raises(BadRequest): obj.query_list() + + +class TestFromEncodedEmpty: + def test_empty_string_means_no_conditions(self, caplog): + # A bare extra_conditions= URL parameter (the results page URL + # rewrite produces those) is not invalid input: no conditions, + # and no warning logged. + import logging + + with caplog.at_level(logging.WARNING, logger="kitconcept.solr"): + conditions = SolrExtraConditions.from_encoded("") + assert conditions.query_list() == [] + assert caplog.records == [] + + def test_none_means_no_conditions(self): + assert SolrExtraConditions.from_encoded(None).query_list() == []