From b9eae385566833546e64606ad4643f4b819d9083 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 14 Jul 2026 11:02:46 +0200 Subject: [PATCH] feat(librarian): send explicit non-empty user on every library-desk request Library-desk is removing its server-side default user, so a request without an explicit tenant will 422 after its next deploy: - New client-level _resolve_user() resolves the tenant (explicit arg or request context) and raises ValueError on an empty/whitespace value BEFORE any bytes hit the wire; all 15 tenant-scoped methods use it - extract_content / extract_content_batch now accept and send the user (query param), matching the rest of the API surface - search_web no longer falls back to a phantom "tatlock-librarian" tenant; it sends the resolved user - health_check stays user-less (public, not tenant-scoped) Tests: parametrized sweep pins the wire contract (user present in params or payload) for every tenant-scoped method, for both context and explicit users; empty-tenant calls are asserted to fail without any HTTP call; the recorded-fixture hybrid contract test now pins user as an explicit query param. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + src/agents/librarian/client.py | 58 ++++++-- tests/agents/librarian/test_client.py | 139 +++++++++++++++++- .../agents/librarian/test_hybrid_contract.py | 15 ++ 4 files changed, 190 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e412cf..92b531e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Explicit tenant on every library-desk request** - the librarian client now resolves and sends the `user` parameter explicitly on every request (library-desk is removing its server-side default; a missing user would 422). The content extraction endpoints now carry the tenant too, `search_web` no longer falls back to a phantom `tatlock-librarian` user, and a client-level assertion rejects an empty/whitespace tenant before any bytes hit the wire. A parametrized sweep pins the wire contract for all 15 tenant-scoped client methods - **Tenant isolation guard** - non-production environments (development/testing) now FORCE the effective tenant to the reserved test tenant `llm_tester` (only `llm_tester` itself or a `test_`-prefixed override is accepted), regardless of `DEFAULT_USER` misconfiguration, at both config resolution and request-context resolution (`get_user()`). Startup refuses (clear error) when a non-production environment is explicitly configured with the production tenant `jpmschweitzer`, and one loud startup log line states the effective/forced tenant - **Conversation context for experts + real-time think messages** - direct delegation (streaming and non-streaming) now passes a trimmed conversation history (last 6 turns) as expert context, so follow-up questions keep their referent; `_stream_direct_delegation` is now an async generator, so butler think messages ("Allow me to consult the archives, sir.") stream BEFORE the research runs instead of after it completes diff --git a/src/agents/librarian/client.py b/src/agents/librarian/client.py index 87d6f00..3d0a98d 100644 --- a/src/agents/librarian/client.py +++ b/src/agents/librarian/client.py @@ -263,6 +263,23 @@ class LibraryDeskClient: ) return self._client + def _resolve_user(self, user: str | None) -> str: + """ + Resolve the effective tenant for a request and require it non-empty. + + Library-desk is removing its server-side default user, so every + request must carry an explicit tenant (a missing user will 422). + An empty tenant is a programming or configuration error - fail + loudly here, before any bytes hit the wire. + """ + effective = user if user is not None else get_user() + if not effective or not effective.strip(): + raise ValueError( + "library-desk request requires a non-empty user (tenant); " + "got an empty value from the caller or request context" + ) + return effective + async def _request_with_retry( self, send: Callable[[], Awaitable[httpx.Response]], @@ -337,7 +354,7 @@ class LibraryDeskClient: Returns: HybridRAGResponse with ranked results and context """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() # The live service requires all limits >= 1 (422 otherwise); @@ -446,7 +463,7 @@ class LibraryDeskClient: Returns: List of matching wiki pages """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() logger.debug("library_desk_wiki_search", query=query, user=user) @@ -478,7 +495,7 @@ class LibraryDeskClient: Returns: WikiPage with full content """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() response = await self._request_with_retry( @@ -509,7 +526,7 @@ class LibraryDeskClient: Returns: List of wiki pages """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() params: dict[str, Any] = {"user": user, "limit": limit} @@ -548,7 +565,7 @@ class LibraryDeskClient: Returns: Created WikiPage """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() payload = { @@ -593,7 +610,7 @@ class LibraryDeskClient: Returns: Updated WikiPage """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() # Build update payload with only provided fields @@ -651,7 +668,7 @@ class LibraryDeskClient: Returns: SmartCreateResponse with page and research metadata """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() payload: dict[str, Any] = { @@ -702,7 +719,7 @@ class LibraryDeskClient: Returns: List of dossiers with page counts """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() response = await self._request_with_retry( @@ -740,7 +757,7 @@ class LibraryDeskClient: Returns: List of matching document chunks with scores """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() payload = { @@ -781,7 +798,7 @@ class LibraryDeskClient: Returns: List of result records """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() payload = { @@ -814,7 +831,7 @@ class LibraryDeskClient: Returns: List of graph nodes """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() params: dict[str, Any] = {"user": user, "limit": limit} @@ -845,7 +862,7 @@ class LibraryDeskClient: Returns: Node with relationships and connected nodes """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() response = await self._request_with_retry( @@ -907,14 +924,14 @@ class LibraryDeskClient: Returns: WebSearchResponse with results and pre-formatted sources """ - user = user or get_user() + user = self._resolve_user(user) client = self._ensure_client() payload = { "query": query, "search_type": search_type, "limit": limit, - "user": user or "tatlock-librarian", + "user": user, } logger.info("library_desk_web_search", query=query, limit=limit) @@ -955,6 +972,7 @@ class LibraryDeskClient: async def extract_content( self, url: str, + user: str | None = None, include_metadata: bool = True, max_length: int = 5000, ) -> ContentExtractionResult: @@ -968,12 +986,14 @@ class LibraryDeskClient: Args: url: URL to extract content from + user: User identifier (defaults to request context) include_metadata: Whether to extract author, date, etc. max_length: Maximum content length Returns: ContentExtractionResult (check .success and .error fields) """ + user = self._resolve_user(user) client = self._ensure_client() payload = { @@ -984,7 +1004,11 @@ class LibraryDeskClient: logger.debug("library_desk_extract_content", url=url) - response = await client.post("/content/extract", json=payload) + response = await client.post( + "/content/extract", + json=payload, + params={"user": user}, + ) response.raise_for_status() data = response.json() @@ -1004,6 +1028,7 @@ class LibraryDeskClient: async def extract_content_batch( self, urls: list[str], + user: str | None = None, include_metadata: bool = True, max_length: int = 2000, ) -> BatchExtractionResponse: @@ -1017,12 +1042,14 @@ class LibraryDeskClient: Args: urls: List of URLs to extract (max 20) + user: User identifier (defaults to request context) include_metadata: Whether to extract author, date, etc. max_length: Maximum content length per URL Returns: BatchExtractionResponse with results and stats """ + user = self._resolve_user(user) client = self._ensure_client() payload = { @@ -1036,6 +1063,7 @@ class LibraryDeskClient: response = await client.post( "/content/extract/batch", json=payload, + params={"user": user}, ) response.raise_for_status() diff --git a/tests/agents/librarian/test_client.py b/tests/agents/librarian/test_client.py index b8086a7..7c8109a 100644 --- a/tests/agents/librarian/test_client.py +++ b/tests/agents/librarian/test_client.py @@ -2,22 +2,23 @@ Tests for the Library-Desk HTTP client. """ -import pytest from unittest.mock import AsyncMock, MagicMock, patch + import httpx +import pytest from src.agents.librarian.client import ( - LibraryDeskClient, + Dossier, + EntityLinking, + GraphNode, HybridRAGResponse, HybridSearchResult, + LibraryDeskClient, + ResearchSummary, + SmartCreateResponse, + VectorSearchResult, WikiPage, WikiSearchResult, - VectorSearchResult, - GraphNode, - Dossier, - SmartCreateResponse, - ResearchSummary, - EntityLinking, ) @@ -549,6 +550,128 @@ class TestSmartCreateWikiPage: assert result.research_summary.web_results == 0 +# All tenant-scoped client methods with minimal call kwargs. Used to sweep +# the explicit-user contract: every library-desk request must carry a +# non-empty user (the service is removing its server-side default). +TENANT_SCOPED_METHODS = [ + ("hybrid_search", {"query": "q"}), + ("search_wiki", {"query": "q"}), + ("get_wiki_page", {"page_id": 1}), + ("list_wiki_pages", {}), + ("create_wiki_page", {"title": "t", "path": "/p", "content": "c"}), + ("update_wiki_page", {"page_id": 1, "content": "c"}), + ("smart_create_wiki_page", {"topic": "t", "tags": ["x"]}), + ("list_dossiers", {}), + ("semantic_search", {"query": "q"}), + ("query_graph", {"cypher_query": "MATCH (n) RETURN n"}), + ("list_graph_nodes", {}), + ("get_graph_node", {"node_id": "n1"}), + ("search_web", {"query": "q"}), + ("extract_content", {"url": "http://example.com"}), + ("extract_content_batch", {"urls": ["http://example.com"]}), +] + +# One permissive response body that satisfies every method's parser +# (extra keys are ignored by the pydantic models). +UNIVERSAL_RESPONSE = { + "id": 1, + "path": "/p", + "title": "T", + "results": [], + "pages": [], + "dossiers": [], + "records": [], + "nodes": [], + "keywords": [], + "page": {"id": 1, "path": "/p", "title": "T"}, + "result": {"url": "http://example.com", "success": True}, +} + + +@pytest.mark.unit +class TestExplicitUserContract: + """Every library-desk request sends a non-empty user explicitly.""" + + def _wire_client(self): + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = UNIVERSAL_RESPONSE + mock_response.raise_for_status = MagicMock() + + mock_httpx = AsyncMock(spec=httpx.AsyncClient) + mock_httpx.get.return_value = mock_response + mock_httpx.post.return_value = mock_response + mock_httpx.put.return_value = mock_response + + client = LibraryDeskClient(base_url="http://test:8089", api_key="k") + client._client = mock_httpx + return client, mock_httpx + + def _sent_user(self, mock_httpx) -> str: + """Extract the user sent on the single outgoing request.""" + calls = ( + mock_httpx.get.call_args_list + + mock_httpx.post.call_args_list + + mock_httpx.put.call_args_list + ) + assert len(calls) == 1, "expected exactly one outgoing request" + kwargs = calls[0].kwargs + params = kwargs.get("params") or {} + payload = kwargs.get("json") or {} + return params.get("user") or payload.get("user") or "" + + @pytest.mark.asyncio + @pytest.mark.parametrize("method_name,kwargs", TENANT_SCOPED_METHODS) + async def test_user_from_context_is_sent_on_the_wire( + self, method_name, kwargs + ): + """With no explicit user, the context user is resolved and sent.""" + client, mock_httpx = self._wire_client() + + with patch( + "src.agents.librarian.client.get_user", return_value="llm_tester" + ): + await getattr(client, method_name)(**kwargs) + + assert self._sent_user(mock_httpx) == "llm_tester" + + @pytest.mark.asyncio + @pytest.mark.parametrize("method_name,kwargs", TENANT_SCOPED_METHODS) + async def test_explicit_user_is_sent_on_the_wire(self, method_name, kwargs): + """An explicitly passed user is sent unchanged.""" + client, mock_httpx = self._wire_client() + + await getattr(client, method_name)(user="test_phase_b", **kwargs) + + assert self._sent_user(mock_httpx) == "test_phase_b" + + @pytest.mark.asyncio + @pytest.mark.parametrize("method_name,kwargs", TENANT_SCOPED_METHODS) + async def test_empty_context_user_fails_before_any_request( + self, method_name, kwargs + ): + """An empty resolved user raises before any bytes hit the wire.""" + client, mock_httpx = self._wire_client() + + with patch("src.agents.librarian.client.get_user", return_value=""): + with pytest.raises(ValueError, match="non-empty user"): + await getattr(client, method_name)(**kwargs) + + mock_httpx.get.assert_not_called() + mock_httpx.post.assert_not_called() + mock_httpx.put.assert_not_called() + + @pytest.mark.asyncio + async def test_explicit_whitespace_user_is_rejected(self): + """A whitespace-only explicit user is rejected.""" + client, mock_httpx = self._wire_client() + + with pytest.raises(ValueError, match="non-empty user"): + await client.hybrid_search("q", user=" ") + + mock_httpx.post.assert_not_called() + + @pytest.mark.unit class TestNewResponseModels: """Tests for new response models.""" diff --git a/tests/agents/librarian/test_hybrid_contract.py b/tests/agents/librarian/test_hybrid_contract.py index 14ccb80..9314eca 100644 --- a/tests/agents/librarian/test_hybrid_contract.py +++ b/tests/agents/librarian/test_hybrid_contract.py @@ -150,6 +150,21 @@ class TestHybridRAGContract: assert config["enable_documents"] is False assert config["enable_volatile"] is False + @pytest.mark.asyncio + async def test_user_always_sent_as_query_param( + self, client_with_recorded_response + ): + """The tenant is always sent explicitly - library-desk is removing + its server-side default, so a missing user would 422.""" + await client_with_recorded_response.hybrid_search( + "home server infrastructure", user="testuser" + ) + + params = client_with_recorded_response._client.post.call_args.kwargs[ + "params" + ] + assert params["user"] == "testuser" + @pytest.mark.asyncio async def test_source_counts_and_timing_parsed( self, client_with_recorded_response