From 8bc9c3a7633efbfce61c6b8565e1b0218ea5ad41 Mon Sep 17 00:00:00 2001 From: jwvanderstam Date: Wed, 11 Nov 2026 00:10:00 +0100 Subject: [PATCH 1/2] fix(chat): a question the planner splits keeps the chat's restrictions Multi-hop retrieval passed neither source_ids nor additional_workspace_ids to its sub-questions, so a chat narrowed to chosen documents searched the whole workspace whenever the planner judged a question multi-hop, and extra workspaces were silently dropped (ADR-6, recorded as a live defect). Its citations also lacked knowledge_tier and low_relevance, so a global contribution was cited as a workspace document. Both paths now build citations with one helper. Workspace isolation itself was never affected. Co-Authored-By: Claude Opus 5.5 --- .claude/rules/file-map.md | 1 + CHANGELOG.md | 8 ++ docs/ADR.md | 15 +-- src/services/chat.py | 56 ++++++----- .../test_multi_hop_keeps_the_request_scope.py | 98 +++++++++++++++++++ 5 files changed, 144 insertions(+), 34 deletions(-) create mode 100644 tests/unit/test_multi_hop_keeps_the_request_scope.py diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index 8a221bc0..cc056cfd 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -219,6 +219,7 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `tests/unit/test_upload_isolation_and_size.py` | P1-1/P1-5 — each upload stages into its own directory so two of the same filename cannot collide or delete each other (H4), `MAX_CONTENT_LENGTH` is enforced while the body streams (M2), and more than one uvicorn worker aborts the boot (M7) | | `tests/unit/test_aggregator_carries_request_scope.py` | With the aggregator on, retrieval runs in the caller's workspace: the chat route binds the scope before retrieval, not only for the stream, and each `ThreadPoolExecutor` job runs in a copy of the caller's context. Both were missing, and every `local_docs` job came back empty | | `tests/unit/test_chat_additional_workspaces_authorised.py` | Chat's `additional_workspace_ids` are authorised like the primary workspace — a member may name their own, an admin any, a workspace API key none — and one foreign id refuses the request before any retrieval; the guard used to authorise the primary workspace only | +| `tests/unit/test_multi_hop_keeps_the_request_scope.py` | ADR-6 — a question the planner splits passes the chat's `source_ids` and `additional_workspace_ids` to every sub-question, and its citations name their tier and weak relevance like the single-query path's | | `tests/unit/test_chat_conversation_is_authorised.py` | R-3 (QP-9 review) — a chat naming a conversation outside the authorised scope is a 404 before any retrieval or write, the lookup uses that scope, and the caller's own conversation is still answered | | `tests/integration/test_rating_ids_are_in_scope.py` | R-8 (QP-9 review) — against Postgres: the caller's own and the global tier's chunks are citable, a foreign, retired or archived one is not, nor a global one where the tier is closed; a message is found only in a live conversation of the scope | | `tests/unit/test_frontend_feedback_workspace.py` | R-8 — a rating sends the active workspace, so the server checks its ids there rather than in the default one | diff --git a/CHANGELOG.md b/CHANGELOG.md index 1840adf3..fbfb87c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ OpenAI, `https://api.openai.com/v1`) and drop any `provider/` prefix from `CLOUD proxy server LocalChat never ran, and every fixed version pulls `boto3` into the image. The `pip-audit` suppression for it is removed; `SECURITY.md` item 12 is resolved. +### Fixed + +- **A question the planner splits keeps the chat's restrictions.** Multi-hop retrieval + passed neither the chosen sources nor the additional workspaces to its sub-questions, so + a chat narrowed to some documents searched the whole workspace whenever the planner + judged a question multi-hop (ADR-6 recorded it). Its citations also lacked the tier, so + a global contribution was cited as a workspace document. Workspace isolation itself held. + ### Changed - **The cloud fallback talks to an OpenAI-compatible endpoint directly.** diff --git a/docs/ADR.md b/docs/ADR.md index 0698e449..8ad0a350 100644 --- a/docs/ADR.md +++ b/docs/ADR.md @@ -443,13 +443,14 @@ settles it. - Merging an orchestration experiment to the default path on a qualitative impression. **Consequences to execute.** These make the code agree with the decision; none needs P2-3. -- ⬜ **Multi-hop retrieval drops the caller's scoping** (a live defect, found while writing - this record). `get_rag_context_multi_hop` takes neither `source_ids` nor - `additional_workspace_ids`, so a user who narrowed the chat to specific sources and asks a - ≥ 7-word question the planner judges multi-hop gets retrieval across the whole workspace, - and a request spanning additional workspaces silently loses them. Workspace isolation - itself holds; `workspace_id` is still passed. Fix first: it is a correctness bug on the - default path, independent of everything else here. +- ✅ (2026-11-11) **Multi-hop retrieval dropped the caller's scoping** (a live defect, found + while writing this record). `get_rag_context_multi_hop` took neither `source_ids` nor + `additional_workspace_ids`, so a user who narrowed the chat to specific sources and asked + a ≥ 7-word question the planner judged multi-hop got retrieval across the whole + workspace, and a request spanning additional workspaces silently lost them. Workspace + isolation itself held; `workspace_id` was still passed. Fixed: every sub-question now gets + both, and its citations carry `knowledge_tier` and `low_relevance` like the single-query + path's (`tests/unit/test_multi_hop_keeps_the_request_scope.py`). - ⬜ The aggregator path passes `filename_filter=[]` and receives no `source_ids` or `additional_workspace_ids` (`services/chat.py`, `retrieve_via_aggregator`). Latent while the flag is off; enabling it would change what gets searched. Wire them through or delete diff --git a/src/services/chat.py b/src/services/chat.py index 27de3c19..1dff2e74 100644 --- a/src/services/chat.py +++ b/src/services/chat.py @@ -136,26 +136,27 @@ def get_rag_context( logger.warning("[RAG] No chunks retrieved from documents") return "", [] - sources = [ - { - "filename": r.filename, - "chunk_index": r.chunk_index, - "page_number": r.metadata.get("page_number"), - "section_title": r.metadata.get("section_title"), - "chunk_id": r.chunk_id, - # Carried so the interface can say the sources are weak rather than - # presenting them as if they were not. This is what a citation to an - # unrelated document looked like from the outside: indistinguishable. - "low_relevance": bool(r.metadata.get("low_relevance")), - # "global" when the chunk came from a contribution, so a citation says which - # tier it is (GKB-3); a workspace document is "local". - "knowledge_tier": r.metadata.get("knowledge_tier", "local"), - } - for r in results - ] + sources = [_citation(r) for r in results] return doc_processor.format_context_for_llm(results, max_length=config.MAX_CONTEXT_LENGTH), sources +def _citation(r: RetrievalResult) -> dict[str, Any]: + return { + "filename": r.filename, + "chunk_index": r.chunk_index, + "page_number": r.metadata.get("page_number"), + "section_title": r.metadata.get("section_title"), + "chunk_id": r.chunk_id, + # Carried so the interface can say the sources are weak rather than + # presenting them as if they were not. This is what a citation to an + # unrelated document looked like from the outside: indistinguishable. + "low_relevance": bool(r.metadata.get("low_relevance")), + # "global" when the chunk came from a contribution, so a citation says which + # tier it is (GKB-3); a workspace document is "local". + "knowledge_tier": r.metadata.get("knowledge_tier", "local"), + } + + def get_web_context(message: str) -> tuple[str, list[dict[str, Any]]]: """Return (context_for_prompt, source_entries) for a web-enhanced query.""" if config.MCP_ENABLED: @@ -341,7 +342,10 @@ def retrieve_contexts( if plan is not None and plan.is_multi_hop: local_context, sources = get_rag_context_multi_hop( plan.sub_questions, doc_processor, filename_filter, - chunks_retrieved_ref=chunks_retrieved_ref, scope=scope, + chunks_retrieved_ref=chunks_retrieved_ref, + additional_workspace_ids=additional_workspace_ids, + source_ids=source_ids, + scope=scope, knowledge_scope=knowledge_scope, ) else: @@ -373,6 +377,8 @@ def get_rag_context_multi_hop( doc_processor: Any, filename_filter: list[str], chunks_retrieved_ref: list[int] | None = None, + additional_workspace_ids: list[str] | None = None, + source_ids: list[str] | None = None, *, scope: Scope, knowledge_scope: KnowledgeScope = "local", @@ -384,7 +390,10 @@ def get_rag_context_multi_hop( futures = { executor.submit( doc_processor.retrieve_context, q, - filename_filter=filename_filter, scope=scope, + filename_filter=filename_filter, + additional_workspace_ids=additional_workspace_ids, + source_ids=source_ids or [], + scope=scope, knowledge_scope=knowledge_scope, ): q for q in sub_questions @@ -410,14 +419,7 @@ def get_rag_context_multi_hop( local_context = doc_processor.format_context_for_llm(merged, max_length=config.MAX_CONTEXT_LENGTH) sources = [ - { - "filename": r.filename, - "chunk_index": r.chunk_index, - "page_number": r.metadata.get("page_number"), - "section_title": r.metadata.get("section_title"), - "chunk_id": r.chunk_id, - "combined_score": r.metadata.get("combined_score"), - } + {**_citation(r), "combined_score": r.metadata.get("combined_score")} for r in merged ] return local_context, sources diff --git a/tests/unit/test_multi_hop_keeps_the_request_scope.py b/tests/unit/test_multi_hop_keeps_the_request_scope.py new file mode 100644 index 00000000..049c233e --- /dev/null +++ b/tests/unit/test_multi_hop_keeps_the_request_scope.py @@ -0,0 +1,98 @@ +"""A question the planner splits keeps every restriction the single-query path keeps. + +ADR-6 recorded it as a live defect: `get_rag_context_multi_hop` took neither +`source_ids` nor `additional_workspace_ids`, so a chat narrowed to chosen sources searched +the whole workspace whenever the planner judged the question multi-hop, and a chat spanning +extra workspaces silently lost them. Its citations also lacked `knowledge_tier`, so a +global contribution in a multi-hop answer was cited as a workspace document (GKB-3). +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from src.rag.planner import QueryPlan +from src.rag.retrieval import RetrievalResult +from src.services import chat + +pytestmark = pytest.mark.unit + +WS = "11111111-1111-1111-1111-111111111111" +OTHER_WS = "22222222-2222-2222-2222-222222222222" + + +def _processor(results: list[RetrievalResult]) -> MagicMock: + processor = MagicMock() + processor.retrieve_context.return_value = results + processor.format_context_for_llm.return_value = "context" + return processor + + +def _result(chunk_id: int, tier: str = "local", low: bool = False) -> RetrievalResult: + return RetrievalResult( + chunk_text="text", filename=f"doc{chunk_id}.md", chunk_index=0, + similarity=0.9 - chunk_id / 100, + metadata={"knowledge_tier": tier, "low_relevance": low}, chunk_id=chunk_id, + ) + + +def _retrieve(processor: MagicMock, plan: QueryPlan, **kwargs: Any) -> list[dict[str, Any]]: + fields = {"message": "a question", "use_rag": True, "enhance": False, + "conversation_id": None, "knowledge_scope": kwargs.pop("knowledge_scope", "local")} + db = MagicMock() + db.conversations.document_filter.return_value = [] + _, _, sources, _ = chat.retrieve_contexts( + fields, processor, db, [0], plan=plan, scope=WS, **kwargs, + ) + return sources + + +MULTI_HOP = QueryPlan(sub_questions=["first part?", "second part?"], estimated_hops=2) + + +class TestEverySubQuestionKeepsTheRequestScope: + def test_chosen_sources_reach_every_sub_question(self): + processor = _processor([]) + _retrieve(processor, MULTI_HOP, source_ids=["src-1", "src-2"]) + assert [c.kwargs["source_ids"] for c in processor.retrieve_context.call_args_list] == [ + ["src-1", "src-2"], ["src-1", "src-2"], + ] + + def test_additional_workspaces_reach_every_sub_question(self): + processor = _processor([]) + _retrieve(processor, MULTI_HOP, additional_workspace_ids=[OTHER_WS]) + assert [c.kwargs["additional_workspace_ids"] for c in processor.retrieve_context.call_args_list] == [ + [OTHER_WS], [OTHER_WS], + ] + + def test_without_either_the_sub_questions_search_the_workspace_alone(self): + processor = _processor([]) + _retrieve(processor, MULTI_HOP) + assert [ + (c.kwargs["source_ids"], c.kwargs["additional_workspace_ids"]) + for c in processor.retrieve_context.call_args_list + ] == [([], None), ([], None)] + + def test_the_single_query_path_still_passes_the_same_restriction(self): + processor = _processor([]) + _retrieve(processor, QueryPlan(sub_questions=["one?"], estimated_hops=1), + source_ids=["src-1"], additional_workspace_ids=[OTHER_WS]) + call = processor.retrieve_context.call_args + assert (call.kwargs["source_ids"], call.kwargs["additional_workspace_ids"]) == ( + ["src-1"], [OTHER_WS], + ) + + +class TestAMultiHopCitationSaysWhatItIs: + def test_a_global_chunk_is_cited_as_global_and_a_workspace_one_as_local(self): + processor = _processor([_result(1, tier="global"), _result(2, tier="local")]) + sources = _retrieve(processor, MULTI_HOP, knowledge_scope="hybrid") + assert {s["chunk_id"]: s["knowledge_tier"] for s in sources} == {1: "global", 2: "local"} + + def test_a_weak_chunk_is_marked_low_relevance(self): + processor = _processor([_result(1, low=True), _result(2, low=False)]) + sources = _retrieve(processor, MULTI_HOP) + assert {s["chunk_id"]: s["low_relevance"] for s in sources} == {1: True, 2: False} From d501da0683543a0834803cb22e17d0adbfdcdef8 Mon Sep 17 00:00:00 2001 From: jwvanderstam Date: Wed, 11 Nov 2026 00:16:53 +0100 Subject: [PATCH 2/2] test(chat): pin the retrieval path and assert all five restrictions per sub-question Answers the two Low findings of the #538 review: the tests now set AGGREGATOR_AGENT_ENABLED and MCP_ENABLED off themselves, and one test asserts scope, knowledge_scope, filename_filter, source_ids and additional_workspace_ids on every sub-question call. Co-Authored-By: Claude Opus 5.5 --- .../test_multi_hop_keeps_the_request_scope.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/test_multi_hop_keeps_the_request_scope.py b/tests/unit/test_multi_hop_keeps_the_request_scope.py index 049c233e..73f314f6 100644 --- a/tests/unit/test_multi_hop_keeps_the_request_scope.py +++ b/tests/unit/test_multi_hop_keeps_the_request_scope.py @@ -14,12 +14,21 @@ import pytest +from src import config from src.rag.planner import QueryPlan from src.rag.retrieval import RetrievalResult from src.services import chat pytestmark = pytest.mark.unit + +@pytest.fixture(autouse=True) +def _direct_retrieval(monkeypatch): + # Either flag sends retrieval down another path before multi-hop is reached, and the + # single-query control would then pass for the wrong reason. + monkeypatch.setattr(config, "AGGREGATOR_AGENT_ENABLED", False) + monkeypatch.setattr(config, "MCP_ENABLED", False) + WS = "11111111-1111-1111-1111-111111111111" OTHER_WS = "22222222-2222-2222-2222-222222222222" @@ -76,6 +85,22 @@ def test_without_either_the_sub_questions_search_the_workspace_alone(self): for c in processor.retrieve_context.call_args_list ] == [([], None), ([], None)] + def test_every_sub_question_gets_all_five_restrictions_of_the_request(self): + processor = _processor([]) + db_filter = ["chosen.md"] + fields = {"message": "a question", "use_rag": True, "enhance": False, + "conversation_id": "c1", "knowledge_scope": "hybrid"} + db = MagicMock() + db.conversations.document_filter.return_value = db_filter + chat.retrieve_contexts(fields, processor, db, [0], plan=MULTI_HOP, scope=WS, + source_ids=["src-1"], additional_workspace_ids=[OTHER_WS]) + assert [ + {k: c.kwargs[k] for k in ("scope", "knowledge_scope", "filename_filter", + "source_ids", "additional_workspace_ids")} + for c in processor.retrieve_context.call_args_list + ] == [{"scope": WS, "knowledge_scope": "hybrid", "filename_filter": db_filter, + "source_ids": ["src-1"], "additional_workspace_ids": [OTHER_WS]}] * 2 + def test_the_single_query_path_still_passes_the_same_restriction(self): processor = _processor([]) _retrieve(processor, QueryPlan(sub_questions=["one?"], estimated_hops=1),