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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbFZyDvYksazX6nYQYZ67L
This commit is contained in:
+3
-1
@@ -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
|
||||
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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})
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user