From 8cf36099489bce474706fe20eda99247cecea742 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 14 Jul 2026 11:59:15 +0200 Subject: [PATCH] fix(librarian): quiet healthy-search coverage notes, tag clearing, strict result pairing Phase A review minors: - Coverage note: source_status (when present) is now used exclusively; the source_counts-absence fallback only considers the optional legs the request explicitly enabled (web/documents/volatile). library-desk computes source_counts from the final top-N fused results only, so absence of the always-on vector/graph legs is normal ranking behavior - the old heuristic warned on virtually every healthy search - update_wiki_page: the empty-list tags sentinel (leave unchanged) made clearing all tags impossible; pass exactly ["__CLEAR__"] to send an empty tag list, documented in the docstring for the local model - Text-delegation parallel fallback: zip(..., strict=True) with an explicit count-mismatch guard so results can never be silently attributed to the wrong agent Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QbFZyDvYksazX6nYQYZ67L --- CHANGELOG.md | 4 +- src/agents/librarian/tools.py | 35 ++++++++--- src/responses/service.py | 17 +++++- .../agents/librarian/test_hybrid_contract.py | 16 ++++- tests/agents/librarian/test_tools.py | 59 +++++++++++++++++++ 5 files changed, 119 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f05398..23d10b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,10 +20,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **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 - **Bounded retries and connection reuse for library-desk** - GETs and the read-only `POST /query/*` and `POST /rag/search` endpoints retry once (2 attempts, short backoff) on transport errors and retryable 5xx; wiki writes are never retried. The client now honors `LIBRARY_DESK_TIMEOUT` instead of hardcoded 60s/30s, a librarian run holds one shared HTTP connection instead of constructing a client per tool call, and read tools raise `ModelRetry` on transient HTTP errors so the agent's retry budget engages - **One librarian timeout budget** - new `LIBRARIAN_TIMEOUT` (default 180s) enforced with `asyncio.wait_for` inside `delegate_to_librarian`, capping the previously uncapped live paths (steward direct delegation and streaming). The Ollama provider's AsyncOpenAI client now carries an explicit `OLLAMA_TIMEOUT` instead of the SDK's ~600s default, and the contradictory unused 60s default in `AgentRequest.timeout_seconds` was removed (None defers to the configured budget) -- **Search degradation signaling** - The librarian client parses `source_counts` (plus the additive `source_status`/`degraded` fields when a newer library-desk sends them; absence is tolerated), and `hybrid_search` appends a one-line coverage note when a search is degraded or an enabled source leg contributed nothing, so outages are visible to the model and the user +- **Search degradation signaling** - The librarian client parses `source_counts` (plus the additive `source_status`/`degraded` fields when a newer library-desk sends them; absence is tolerated), and `hybrid_search` appends a one-line coverage note when a search is degraded or an enabled source leg contributed nothing, so outages are visible to the model and the user. When `source_status` is present it is used exclusively; without it, count-absence is only inferred for the optional legs the request explicitly enabled (web/documents/volatile) - never the always-on vector/graph legs, whose absence from the top-N counts is normal ranking behavior, so healthy searches no longer emit warnings ### Fixed +- **Clearing all wiki-page tags is possible again** - the Ollama-safe empty-list sentinel in `update_wiki_page` means "leave unchanged", which made it impossible to remove all tags; passing exactly `["__CLEAR__"]` now sends an empty tag list to library-desk (documented in the tool docstring for the local model) +- **Text-delegation fallback pairs results strictly** - the parallel branch now verifies `asyncio.gather` returned one result per parsed delegation (`zip(..., strict=True)`); a count mismatch fails loudly with a curated apology instead of silently attributing outputs to the wrong agent - **Ollama-safe librarian tool schemas** - `update_wiki_page` and `smart_create_wiki_page` no longer use `X | None` parameters (Ollama's OpenAI-compatible API mishandles `anyOf[X, null]`); empty-string/empty-list sentinels are translated to `None` inside the tools, matching the biographer pattern. A snapshot test pins every librarian tool schema to contain no nullable `anyOf` - **Honest expert failures** - `run_librarian`/`run_librarian_stream` now raise a structured `AgentError` instead of returning error text as if it were research output, so delegation correctly reports `success=False` and the streaming error branch is reachable. Failures surface to the user as curated butler-toned sentences; exception detail (including internal URLs) stays in the logs only. Librarian tool errors no longer leak `str(e)` into synthesis diff --git a/src/agents/librarian/tools.py b/src/agents/librarian/tools.py index 84a9a49..2c2a54f 100644 --- a/src/agents/librarian/tools.py +++ b/src/agents/librarian/tools.py @@ -55,8 +55,13 @@ def _coverage_note( enabled source leg contributed nothing, so outages stay visible to the model and the user instead of silently narrowing results. - Prefers the additive source_status/degraded contract when present; - falls back to inferring silent legs from source_counts. + When the additive source_status/degraded contract is present it is + authoritative and used EXCLUSIVELY - no count heuristics. Without + it, absence from source_counts is only inferred for the optional + legs this request explicitly enabled (web/documents/volatile); + the always-on wiki legs (vector/graph) are never inferred, because + source_counts only tallies the sources of the final top-N fused + results, so their absence is normal ranking behavior, not an outage. """ if response.source_status: failed = sorted( @@ -80,7 +85,9 @@ def _coverage_note( # Older library-desk without per-source reporting - nothing to infer return "" - expected = {"vector", "graph"} + # Only legs the request explicitly enabled; never vector/graph (their + # absence from the top-N counts is healthy, see docstring) + expected = set() if include_web: expected.add("web") if include_documents: @@ -88,8 +95,8 @@ def _coverage_note( if include_volatile: expected.add("volatile") - # Normalize count keys to leg names (document/documents, wiki/vector) - aliases = {"document": "documents", "wiki": "vector"} + # Normalize count keys to leg names (document/documents) + aliases = {"document": "documents"} reported = {aliases.get(key, key) for key in response.source_counts} missing = sorted(expected - reported) if missing: @@ -791,6 +798,9 @@ async def read_urls_batch( # Wiki Write Operations # ============================================================================ +CLEAR_TAGS_SENTINEL = "__CLEAR__" + + async def update_wiki_page( page_id: int, content: str = "", @@ -817,7 +827,9 @@ async def update_wiki_page( page_id: ID of the page to update (get from search_wiki results) content: New markdown content (empty = leave unchanged) title: New title (empty = leave unchanged) - tags: New tag list, replaces existing tags (empty = leave unchanged) + tags: New tag list, replaces existing tags (empty = leave unchanged). + To remove ALL tags from a page, pass exactly ["__CLEAR__"] + (an empty list means "leave unchanged", not "clear") description: New description (empty = leave unchanged) Returns: @@ -826,15 +838,20 @@ async def update_wiki_page( Examples: update_wiki_page(42, content="# Updated Content\\n\\nNew information here") update_wiki_page(42, tags=["projects", "devops"]) # Add to dossiers + update_wiki_page(42, tags=["__CLEAR__"]) # Remove all tags update_wiki_page(42, description="Updated description") """ + # Empty list = leave unchanged; the explicit clear sentinel sends an + # empty tag list to the service, which replaces (clears) all tags. + clear_tags = tags == [CLEAR_TAGS_SENTINEL] + try: async with LibraryDeskClient() as client: page = await client.update_wiki_page( page_id=page_id, content=content if content else None, title=title if title else None, - tags=tags if tags else None, + tags=[] if clear_tags else (tags if tags else None), description=description if description else None, ) @@ -844,7 +861,9 @@ async def update_wiki_page( updated_fields.append("content") if title: updated_fields.append("title") - if tags: + if clear_tags: + updated_fields.append("tags (cleared)") + elif tags: updated_fields.append("tags") if description: updated_fields.append("description") diff --git a/src/responses/service.py b/src/responses/service.py index 00c46f7..f1ebca3 100644 --- a/src/responses/service.py +++ b/src/responses/service.py @@ -178,9 +178,24 @@ async def _handle_text_delegation( ] results = await asyncio.gather(*tasks, return_exceptions=True) + # asyncio.gather returns exactly one item per task; a length + # mismatch would mean results are attributed to the wrong + # agent, so fail loudly instead of mispairing silently. + if len(results) != len(matches): + logger.error( + "delegation_result_count_mismatch", + expected=len(matches), + got=len(results), + conversation_id=conversation_id, + ) + return ( + "I apologize, sir. I was unable to complete the " + "requested delegations." + ) + # Combine results (failures carry curated user-safe sentences) summaries = [] - for (agent, task), item in zip(matches, results, strict=False): + for (agent, task), item in zip(matches, results, strict=True): agent_name = agent.lower() if isinstance(item, BaseException): logger.error( diff --git a/tests/agents/librarian/test_hybrid_contract.py b/tests/agents/librarian/test_hybrid_contract.py index 9314eca..df019be 100644 --- a/tests/agents/librarian/test_hybrid_contract.py +++ b/tests/agents/librarian/test_hybrid_contract.py @@ -264,15 +264,27 @@ class TestCoverageNote: ) assert note == "" - def test_note_when_leg_missing_from_counts(self): + def test_note_when_enabled_leg_missing_from_counts(self): response = self._response(source_counts={"graph": 3, "web": 2}) note = _coverage_note( response, include_web=True, include_documents=True, include_volatile=True ) assert "Coverage note" in note - assert "vector" in note assert "documents" in note assert "volatile" in note + # Always-on wiki legs are never inferred from count absence + assert "vector" not in note + assert "graph" not in note + + def test_wiki_leg_absence_is_not_degradation(self): + """vector/graph missing from top-N counts is healthy ranking, not outage.""" + response = self._response( + source_counts={"web": 2, "documents": 1, "volatile": 1} + ) + note = _coverage_note( + response, include_web=True, include_documents=True, include_volatile=True + ) + assert note == "" def test_disabled_legs_are_not_reported_missing(self): response = self._response(source_counts={"vector": 2, "graph": 1}) diff --git a/tests/agents/librarian/test_tools.py b/tests/agents/librarian/test_tools.py index 032fb47..516e6bd 100644 --- a/tests/agents/librarian/test_tools.py +++ b/tests/agents/librarian/test_tools.py @@ -14,11 +14,14 @@ from src.agents.librarian.client import ( ContentExtractionResult, WebSearchResponse, WebSearchResult, + WikiPage, ) from src.agents.librarian.tools import ( + CLEAR_TAGS_SENTINEL, read_url, read_urls_batch, search_web, + update_wiki_page, ) @@ -427,3 +430,59 @@ class TestWebSearchModels: assert response.total_urls == 2 assert response.successful == 1 assert response.failed == 1 + + +# ============================================================================ +# Wiki Update Tests (tag sentinel behavior) +# ============================================================================ + +@pytest.mark.unit +class TestUpdateWikiPageTagSentinels: + """Empty list leaves tags unchanged; the clear sentinel empties them.""" + + def _page(self, tags: list[str]) -> WikiPage: + return WikiPage(id=42, path="test/page", title="Test Page", tags=tags) + + def _patched_client(self, mock_client): + patcher = patch("src.agents.librarian.tools.LibraryDeskClient") + mock_client_class = patcher.start() + mock_client_class.return_value.__aenter__.return_value = mock_client + mock_client_class.return_value.__aexit__.return_value = None + return patcher + + @pytest.mark.asyncio + async def test_empty_tags_means_leave_unchanged(self, mock_client): + mock_client.update_wiki_page.return_value = self._page(["existing"]) + patcher = self._patched_client(mock_client) + try: + await update_wiki_page(42, content="new content") + finally: + patcher.stop() + + _, kwargs = mock_client.update_wiki_page.call_args + assert kwargs["tags"] is None + + @pytest.mark.asyncio + async def test_clear_sentinel_sends_empty_tag_list(self, mock_client): + mock_client.update_wiki_page.return_value = self._page([]) + patcher = self._patched_client(mock_client) + try: + result = await update_wiki_page(42, tags=[CLEAR_TAGS_SENTINEL]) + finally: + patcher.stop() + + _, kwargs = mock_client.update_wiki_page.call_args + assert kwargs["tags"] == [] + assert "tags (cleared)" in result + + @pytest.mark.asyncio + async def test_real_tags_are_passed_through(self, mock_client): + mock_client.update_wiki_page.return_value = self._page(["a", "b"]) + patcher = self._patched_client(mock_client) + try: + await update_wiki_page(42, tags=["a", "b"]) + finally: + patcher.stop() + + _, kwargs = mock_client.update_wiki_page.call_args + assert kwargs["tags"] == ["a", "b"]