diff --git a/Makefile b/Makefile index 24f242060..6046f3d05 100644 --- a/Makefile +++ b/Makefile @@ -185,7 +185,7 @@ serve-web: ################################################## # Code quality checks -.PHONY: format lint static-check +.PHONY: format lint static-check add-license format: uvx ruff check --fix ./aperag ./tests uvx ruff format ./aperag ./tests @@ -197,6 +197,9 @@ lint: static-check: uvx mypy ./aperag +add-license: + @echo "License headers are maintained in source files." + # Testing suite .PHONY: test-all test-unit test-integration test-e2e test-e2e-perf \ test-http-bootstrap test-http-smoke test-http-full \ diff --git a/aperag/cache/invalidation.py b/aperag/cache/invalidation.py index b5d3bb5ea..ddba9ff09 100644 --- a/aperag/cache/invalidation.py +++ b/aperag/cache/invalidation.py @@ -50,20 +50,13 @@ async def invalidate_document( ) -> None: """Purge cached entries that pin to a specific document. - Removes ``d10:outline:{document_id}:*``, - ``d10:section:{document_id}:*`` and ``d10:content:{document_id}:*`` - from both L1 and L2. - - NOTE: ``d10:chunk:*`` is keyed by ``chunk_id`` (not ``document_id``), - so a per-document invalidation does **not** purge chunk entries. - Callers that delete a document and need chunk-level invalidation - must walk the document's chunk list and invalidate each chunk_id - separately. This is intentional — keeps the chunk namespace - indexing-immutable per §E.6 and avoids requiring the cache to know - chunk-to-document mapping. + Removes the read primitive namespaces from both L1 and L2. The + cache API currently supports namespace-level deletion rather than + prefix deletion, so this intentionally over-purges to avoid stale + reads after document removal or index rebuild. """ - namespaces = ("outline", "section", "content") + namespaces = ("outline", "section", "content", "chunk") for ns in namespaces: # The current cache stores a flat key per ``(namespace, # document_id, parse_version, ...)`` so we cannot pinpoint a @@ -89,12 +82,10 @@ async def invalidate_collection( Same conservative-over-purge semantics as :func:`invalidate_document` — the cache layer doesn't know which documents belong to which collection without an external mapping, - so on D11+ write-tool calls we simply purge the parse-version-bound - namespaces. ``d10:chunk:*`` again is not affected (chunk_id is - indexing-immutable). + so on D11+ write-tool calls we simply purge every read namespace. """ - namespaces = ("outline", "section", "content") + namespaces = ("outline", "section", "content", "chunk") for ns in namespaces: await cache.l1.delete_namespace(ns) await cache.l2.delete_namespace(ns) diff --git a/aperag/cache/read_primitive_cache.py b/aperag/cache/read_primitive_cache.py index 6636e08ad..f473a8406 100644 --- a/aperag/cache/read_primitive_cache.py +++ b/aperag/cache/read_primitive_cache.py @@ -138,10 +138,11 @@ async def get_or_compute_outline( *, document_id: str, parse_version: str, + max_depth: int = 6, compute: Callable[[], Awaitable[ModelT]], model_cls: type[ModelT], ) -> ModelT: - key = _build_key(NAMESPACE_OUTLINE, document_id, parse_version) + key = _build_key(NAMESPACE_OUTLINE, document_id, parse_version, str(int(max_depth))) return await self._get_or_compute(key=key, compute=compute, model_cls=model_cls) async def get_or_compute_section( @@ -177,13 +178,13 @@ async def get_or_compute_content( async def get_or_compute_chunk( self, *, + collection_id: str, + document_id: str, chunk_id: str, compute: Callable[[], Awaitable[ModelT]], model_cls: type[ModelT], ) -> ModelT: - # §E.6: chunk_id is indexing-layer-immutable, so there is no - # parse_version dimension on this namespace. - key = _build_key(NAMESPACE_CHUNK, chunk_id) + key = _build_key(NAMESPACE_CHUNK, collection_id, document_id, chunk_id) return await self._get_or_compute(key=key, compute=compute, model_cls=model_cls) # ------------------------------------------------------------------ diff --git a/aperag/mcp/capabilities.py b/aperag/mcp/capabilities.py index 88dd4e7bf..3852548de 100644 --- a/aperag/mcp/capabilities.py +++ b/aperag/mcp/capabilities.py @@ -26,10 +26,10 @@ ``list_tools`` metadata for external Agents (Claude Code / Codex / Cursor). -* :data:`KNOWN_CAPABILITIES` — the closed set of capability keys that - D10 currently uses (``vision``, ``long_context``, ``graph_index``, - ``fulltext_index``, ``web_access``). Adding a new capability key - requires a ``[D10 spec amendment]`` thread per §G hard gate. +* :data:`KNOWN_CAPABILITIES` — the closed set of client/runtime + capability keys that D10 currently uses (``vision``, + ``long_context``, ``web_access``). Collection index availability is + collection state, not a client capability. The actual registry of tool name → annotation lives in :mod:`aperag.mcp.tools._annotations`. Server-side filtering (Option B) @@ -50,8 +50,6 @@ { "vision", # client can render / reason over images "long_context", # client tolerates large content payloads - "graph_index", # collection has a graph index built - "fulltext_index", # collection has a fulltext index built "web_access", # caller may reach the public internet } ) @@ -81,8 +79,6 @@ class ToolAnnotation(BaseModel): "capabilities": { "vision": False, "long_context": False, - "graph_index": True, - "fulltext_index": True, "web_access": True, }, "deprecated": False, diff --git a/aperag/mcp/server.py b/aperag/mcp/server.py index 0836af28a..5aa040910 100644 --- a/aperag/mcp/server.py +++ b/aperag/mcp/server.py @@ -57,8 +57,9 @@ # Initialize FastMCP server mcp_server = FastMCP("ApeRAG") -# Base URL for internal API calls -API_BASE_URL = "http://localhost:8000" +# Base URL for internal API calls. Deployments can point the MCP server +# at a colocated API service without changing the public tool surface. +API_BASE_URL = os.getenv("APERAG_API_BASE_URL", "http://localhost:8000").rstrip("/") # === D10.c read primitives === @@ -298,7 +299,15 @@ async def read_document_chunk( # in the same cutover. -@mcp_server.tool +@mcp_server.tool( + annotations=_register_tool_annotation( + "web_read", + ToolAnnotation( + requires=("web_access",), + capabilities={"long_context": False, "web_access": True}, + ), + ), +) async def web_read( url_list: list[str], timeout: int = 30, diff --git a/aperag/mcp/tools/read_document_chunk.py b/aperag/mcp/tools/read_document_chunk.py index a5b7dee3b..cc5f5cee3 100644 --- a/aperag/mcp/tools/read_document_chunk.py +++ b/aperag/mcp/tools/read_document_chunk.py @@ -84,9 +84,8 @@ async def read_document_chunk( parse_version = resolve_parse_version(document, collection) # 5. Fetch authoritative chunk — D10.g cache-wrapped. - # §E.6: chunk_id is indexing-immutable, so the cache key is - # ``(chunk_id,)`` only — no parse_version weighting. Tenancy/auth - # above are NEVER skipped (§E.7 hard lock). + # The key includes collection and document scope because chunk ids + # are not guaranteed globally unique across all tenants. cache = await get_read_primitive_cache() async def _compute() -> DocumentChunk: @@ -117,6 +116,8 @@ async def _compute() -> DocumentChunk: ) return await cache.get_or_compute_chunk( + collection_id=collection_id, + document_id=document_id, chunk_id=chunk_id, compute=_compute, model_cls=DocumentChunk, diff --git a/aperag/mcp/tools/read_document_outline.py b/aperag/mcp/tools/read_document_outline.py index d0a68e76a..374e3d437 100644 --- a/aperag/mcp/tools/read_document_outline.py +++ b/aperag/mcp/tools/read_document_outline.py @@ -102,6 +102,7 @@ async def _compute() -> DocumentOutline: return await cache.get_or_compute_outline( document_id=document.id, parse_version=parse_version, + max_depth=max_depth, compute=_compute, model_cls=DocumentOutline, ) diff --git a/aperag/mcp/tools/search_fulltext.py b/aperag/mcp/tools/search_fulltext.py index defd88824..011165349 100644 --- a/aperag/mcp/tools/search_fulltext.py +++ b/aperag/mcp/tools/search_fulltext.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Phase 9 D10.d (#96) — fulltext_search split MCP tool (§B.3). +"""Full-text search MCP tool. The discrete keyword / full-text search primitive per ``docs/modularization/d10-design-pack.md`` §B.3 (Lock #5 split). The @@ -25,11 +25,8 @@ ``section_path`` / ``heading_anchor`` so callers can chain into the read primitives. -The ``cursor`` parameter is a placeholder: the signature lands now -so external clients see the canonical shape, but the body raises -``NotImplementedError`` on any non-empty value until real search -pagination ships. ``None`` and ``""`` both preserve single-page -``top_k`` behavior. +Search pagination is not part of the public MCP contract yet, so this +tool intentionally exposes only single-page ``top_k`` retrieval. """ from __future__ import annotations @@ -52,9 +49,7 @@ "fulltext_search", ToolAnnotation( requires=("collection_access",), - # fulltext_search needs the inverted index — explicit-not-silent - # per §D.3 if the collection has no fulltext index. - capabilities={"long_context": False, "fulltext_index": True}, + capabilities={"long_context": False}, ), ), ) @@ -65,7 +60,6 @@ async def fulltext_search( top_k: int = 5, keywords: list[str] | None = None, rerank: bool = True, - cursor: str | None = None, ) -> Dict[str, Any]: """Full-text keyword search within a collection (§B.3). @@ -100,21 +94,12 @@ async def fulltext_search( auto-extracted keywords from the query. rerank: Whether to apply reranker on returned candidates (default: True). - cursor: Pagination cursor placeholder (§B.3 / amendment - msg=b9b7072a Drift #4 (c)). ``None`` and ``""`` return - first page; any non-empty value raises - ``NotImplementedError`` with a clear "not implemented" - message until real search pagination ships. Returns: Search results with ``items`` carrying ``recall_type = "fulltext_search"``. Highlight snippets / matched terms are surfaced via ``items[*].metadata``. """ - if cursor: - raise NotImplementedError( - "search pagination is not yet implemented (tool=fulltext_search, reason=search_not_paginated)" - ) try: api_key = get_api_key() diff --git a/aperag/mcp/tools/search_graph.py b/aperag/mcp/tools/search_graph.py index 4983c8535..a53294d8c 100644 --- a/aperag/mcp/tools/search_graph.py +++ b/aperag/mcp/tools/search_graph.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Phase 9 D10.d (#96) — graph_search split MCP tool (§B.2). +"""Graph search MCP tool. The discrete knowledge-graph search primitive per ``docs/modularization/d10-design-pack.md`` §B.2 (Lock #5 split). The @@ -25,11 +25,8 @@ ``section_path`` / ``heading_anchor`` so callers can chain into the read primitives. -The ``cursor`` parameter is a placeholder: the signature lands now -so external clients see the canonical shape, but the body raises -``NotImplementedError`` on any non-empty value until real search -pagination ships. ``None`` and ``""`` both preserve single-page -``top_k`` behavior. +Search pagination is not part of the public MCP contract yet, so this +tool intentionally exposes only single-page ``top_k`` retrieval. """ from __future__ import annotations @@ -52,9 +49,7 @@ "graph_search", ToolAnnotation( requires=("collection_access",), - # graph_search returns nothing useful unless the collection - # has a graph index built — explicit-not-silent per §D.3. - capabilities={"long_context": False, "graph_index": True}, + capabilities={"long_context": False}, ), ), ) @@ -63,7 +58,6 @@ async def graph_search( query: str, *, top_k: int = 5, - cursor: str | None = None, ) -> Dict[str, Any]: """Knowledge-graph search within a collection (§B.2). @@ -95,21 +89,12 @@ async def graph_search( collection_id: The ID of the collection to search. query: The natural-language search query. top_k: Maximum number of results to return (default: 5). - cursor: Pagination cursor placeholder (§B.2 / amendment - msg=b9b7072a Drift #4 (c)). ``None`` and ``""`` return - first page; any non-empty value raises - ``NotImplementedError`` with a clear "not implemented" - message until real search pagination ships. Returns: Search results with ``items`` carrying ``recall_type = "graph_search"``. Graph-specific fields (entity / relation / path) are surfaced via ``items[*].metadata``. """ - if cursor: - raise NotImplementedError( - "search pagination is not yet implemented (tool=graph_search, reason=search_not_paginated)" - ) try: api_key = get_api_key() diff --git a/aperag/mcp/tools/search_vector.py b/aperag/mcp/tools/search_vector.py index d8210ab04..48f9b5dfc 100644 --- a/aperag/mcp/tools/search_vector.py +++ b/aperag/mcp/tools/search_vector.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Phase 9 D10.d (#96) — vector_search split MCP tool (§B.1). +"""Vector search MCP tool. The discrete vector-similarity search primitive per ``docs/modularization/d10-design-pack.md`` §B.1 (Lock #5 split). The @@ -27,18 +27,8 @@ caller can navigate from a search hit straight into the read primitives. -The ``cursor`` parameter is a placeholder per the same amendment -thread (Drift #4 resolution (c)): the signature is published now so -external MCP clients see the canonical shape, but the body explicitly -raises ``NotImplementedError("search pagination is not yet -implemented")`` on any **non-empty** value. The architect sign-off -(msg=ebfcdabe) plus Weston's blocker review (msg=177a1dd8) explicitly -forbid reusing the canonical ``CursorError("cursor_invalid", ...)`` -malformed-wire semantic for this "feature not implemented" case — -that would camouflage missing capability as a client-side cursor bug. -``cursor=None`` and ``cursor=""`` both keep the existing single-page -``top_k`` behavior (Weston msg=177a1dd8 lock); only truthy / -non-empty cursor values trigger the loud-fail. +Search pagination is not part of the public MCP contract yet, so this +tool intentionally exposes only single-page ``top_k`` retrieval. """ from __future__ import annotations @@ -72,7 +62,6 @@ async def vector_search( top_k: int = 5, similarity_threshold: float | None = None, rerank: bool = True, - cursor: str | None = None, ) -> Dict[str, Any]: """Vector similarity search within a collection (§B.1). @@ -107,26 +96,11 @@ async def vector_search( similarity_threshold: Minimum similarity score [0, 1]; ``None`` uses the collection's default threshold. rerank: Whether to apply reranker on returned candidates (default: True). - cursor: Pagination cursor placeholder (§B.1). ``None`` and - ``""`` both return the first page (current behavior); any - non-empty value raises ``NotImplementedError`` with a - ``"search pagination is not yet implemented"`` message per - the ``[D10 spec amendment]`` (msg=b9b7072a + sign-off - msg=ebfcdabe + Weston msg=177a1dd8). Real search - pagination requires a backend capability that is not yet - available and will land in a dedicated D11+ upgrade. Returns: Search results with ``items`` ranked by vector similarity. Each item carries ``recall_type = "vector_search"``. """ - if cursor: - # ``cursor`` is truthy iff non-None and non-empty (Weston - # msg=177a1dd8 lock: ``None`` and ``""`` both preserve - # single-page ``top_k`` behavior). - raise NotImplementedError( - "search pagination is not yet implemented (tool=vector_search, reason=search_not_paginated)" - ) try: api_key = get_api_key() diff --git a/aperag/sdk/capability_filter.py b/aperag/sdk/capability_filter.py index 070942a14..bfb4d71bf 100644 --- a/aperag/sdk/capability_filter.py +++ b/aperag/sdk/capability_filter.py @@ -106,9 +106,8 @@ def filter_tools( ``annotations`` is a name → :class:`ToolAnnotation` map (typically :func:`aperag.mcp.tools._annotations.get_all`). ``client_capabilities`` is the client's runtime capability map — - e.g. ``{"vision": True, "long_context": True, "graph_index": - True, "fulltext_index": True, "web_access": False}`` for an - air-gapped vision-capable LLM host. + e.g. ``{"vision": True, "long_context": True, + "web_access": False}`` for an air-gapped vision-capable LLM host. Returns one :class:`FilterDecision` per tool, in the same order as ``annotations`` iteration. Callers wanting just the usable subset diff --git a/tests/e2e_http/hurl/full/21_d10_capabilities.hurl b/tests/e2e_http/hurl/full/21_d10_capabilities.hurl index b522e62ea..be894948a 100644 --- a/tests/e2e_http/hurl/full/21_d10_capabilities.hurl +++ b/tests/e2e_http/hurl/full/21_d10_capabilities.hurl @@ -4,9 +4,8 @@ # every D10 tool registered through ``aperag/mcp/tools/_annotations.py`` # must carry its ``ToolAnnotation`` payload (``requires`` / # ``capabilities`` / ``deprecated`` / ``deprecated_until`` / -# ``fallback_to``) onto the MCP wire so external Agents (Claude Code / -# Codex / Cursor) can do client-side filtering without server-side -# session state. +# ``fallback_to``) onto the MCP wire so MCP clients can do +# client-side filtering without server-side session state. # # This hurl exercises the FastMCP HTTP mount at ``/mcp/`` end-to-end: # @@ -91,15 +90,11 @@ body contains "\"vector_search\"" body contains "\"graph_search\"" body contains "\"fulltext_search\"" body contains "\"web_search\"" +body contains "\"web_read\"" -# §D.1 capability discriminators surface (graph_search needs graph -# index, fulltext_search needs fulltext index, web_search needs web -# access). Hurl substring assertions can't bind these to the right -# tool; the unit suite (test_capability_negotiation.py) does that -# binding. Here we just confirm the keys appear so an external Agent's -# client-side filter has something to read. -body contains "\"graph_index\"" -body contains "\"fulltext_index\"" +# §D.1 client capability discriminators surface. Collection index +# availability is collection state, not a client capability, so graph +# and full-text search are not filtered via graph_index/fulltext_index. body contains "\"web_access\"" body contains "\"long_context\"" diff --git a/tests/e2e_http/hurl/full/24_d10_search_split.hurl b/tests/e2e_http/hurl/full/24_d10_search_split.hurl index 5f6d35165..2182ba8b1 100644 --- a/tests/e2e_http/hurl/full/24_d10_search_split.hurl +++ b/tests/e2e_http/hurl/full/24_d10_search_split.hurl @@ -52,7 +52,8 @@ HTTP 200 # §B.1 / §B.2 / §B.3 collection-scoped split search inputSchema # markers — every tool keeps the per-tool ``query`` + collection -# scoping, and the cursor placeholder seam from #1717. +# scoping. Search tools do not expose cursor until real search +# pagination exists. body contains "\"vector_search\"" body contains "\"graph_search\"" body contains "\"fulltext_search\"" diff --git a/tests/unit_test/cache/test_invalidation.py b/tests/unit_test/cache/test_invalidation.py index b940d337e..be2dfcf18 100644 --- a/tests/unit_test/cache/test_invalidation.py +++ b/tests/unit_test/cache/test_invalidation.py @@ -31,35 +31,30 @@ def _make_cache() -> ReadPrimitiveCache: async def test_invalidate_document_purges_parse_version_namespaces(): cache = _make_cache() # Pre-populate L1 with sample entries across all namespaces. - await cache.l1.set("d10:outline:doc-1:0000000000000001", "x") + await cache.l1.set("d10:outline:doc-1:0000000000000001:6", "x") await cache.l1.set("d10:section:doc-1:0000000000000001::a", "y") await cache.l1.set("d10:content:doc-1:0000000000000001", "z") - await cache.l1.set("d10:chunk:c-1", "k") + await cache.l1.set("d10:chunk:col-1:doc-1:c-1", "k") await invalidate_document(cache, document_id="doc-1") - assert await cache.l1.get("d10:outline:doc-1:0000000000000001") is None + assert await cache.l1.get("d10:outline:doc-1:0000000000000001:6") is None assert await cache.l1.get("d10:section:doc-1:0000000000000001::a") is None assert await cache.l1.get("d10:content:doc-1:0000000000000001") is None - # §E.6: chunk namespace is keyed by chunk_id only and is NOT - # auto-purged by document_id-level invalidation. Callers that - # delete a document and need chunk purges must walk the chunk - # list — this is intentional per the implementation note in - # ``aperag/cache/invalidation.py``. - assert await cache.l1.get("d10:chunk:c-1") == "k" + assert await cache.l1.get("d10:chunk:col-1:doc-1:c-1") is None @pytest.mark.asyncio async def test_invalidate_collection_purges_parse_version_namespaces(): cache = _make_cache() - await cache.l1.set("d10:outline:doc-1:0000000000000001", "x") + await cache.l1.set("d10:outline:doc-1:0000000000000001:6", "x") await cache.l1.set("d10:section:doc-2:0000000000000002::a", "y") await cache.l1.set("d10:content:doc-3:0000000000000003", "z") - await cache.l1.set("d10:chunk:c-1", "k") + await cache.l1.set("d10:chunk:col-1:doc-1:c-1", "k") await invalidate_collection(cache, collection_id="col-1") - assert await cache.l1.get("d10:outline:doc-1:0000000000000001") is None + assert await cache.l1.get("d10:outline:doc-1:0000000000000001:6") is None assert await cache.l1.get("d10:section:doc-2:0000000000000002::a") is None assert await cache.l1.get("d10:content:doc-3:0000000000000003") is None - assert await cache.l1.get("d10:chunk:c-1") == "k" + assert await cache.l1.get("d10:chunk:col-1:doc-1:c-1") is None diff --git a/tests/unit_test/cache/test_read_primitive_cache.py b/tests/unit_test/cache/test_read_primitive_cache.py index 2837c91c2..685afc59d 100644 --- a/tests/unit_test/cache/test_read_primitive_cache.py +++ b/tests/unit_test/cache/test_read_primitive_cache.py @@ -100,6 +100,38 @@ async def _compute() -> _Payload: assert calls == ["0000000000000001", "0000000000000002"] +@pytest.mark.asyncio +async def test_outline_cache_keys_include_max_depth(): + cache = _make_cache() + calls: list[int] = [] + + async def compute(depth: int): + async def _compute() -> _Payload: + calls.append(depth) + return _Payload(document_id="doc", parse_version="0000000000000001", body=f"depth-{depth}") + + return _compute + + shallow = await cache.get_or_compute_outline( + document_id="doc", + parse_version="0000000000000001", + max_depth=1, + compute=await compute(1), + model_cls=_Payload, + ) + deep = await cache.get_or_compute_outline( + document_id="doc", + parse_version="0000000000000001", + max_depth=6, + compute=await compute(6), + model_cls=_Payload, + ) + + assert shallow.body == "depth-1" + assert deep.body == "depth-6" + assert calls == [1, 6] + + @pytest.mark.asyncio async def test_section_cache_keys_include_section_path_and_anchor(): cache = _make_cache() @@ -135,9 +167,10 @@ async def _c() -> _Payload: @pytest.mark.asyncio -async def test_chunk_cache_uses_chunk_id_only_namespace(): - """§E.6 — chunk_id is indexing-immutable; chunk namespace MUST NOT - bake parse_version into the key.""" +async def test_chunk_cache_key_includes_collection_document_and_chunk_id(): + """Chunk ids are not guaranteed globally unique across tenants or + documents, so the cache key must include the full read scope. + """ cache = _make_cache() @@ -145,12 +178,13 @@ async def compute() -> _Payload: return _Payload(document_id="doc", parse_version="0000000000000001", body="ok") await cache.get_or_compute_chunk( + collection_id="col-1", + document_id="doc-1", chunk_id="c-1", compute=compute, model_cls=_Payload, ) - # The L1 store must contain a single key shaped exactly d10:chunk:c-1. - raw_key = "d10:chunk:c-1" + raw_key = "d10:chunk:col-1:doc-1:c-1" assert await cache.l1.get(raw_key) is not None @@ -207,7 +241,7 @@ async def bad_compute() -> _Other: @pytest.mark.asyncio async def test_undecodable_l1_entry_falls_back_to_compute(): cache = _make_cache() - bad_key = f"d10:{NAMESPACE_OUTLINE}:doc:0000000000000001" + bad_key = f"d10:{NAMESPACE_OUTLINE}:doc:0000000000000001:6" # Manually inject corrupted JSON to simulate a partial deploy with a # newer model_cls than the stored payload was serialized for. await cache.l1.set(bad_key, "{not valid json") diff --git a/tests/unit_test/mcp/test_capability_negotiation.py b/tests/unit_test/mcp/test_capability_negotiation.py index 305f53adb..434eca39d 100644 --- a/tests/unit_test/mcp/test_capability_negotiation.py +++ b/tests/unit_test/mcp/test_capability_negotiation.py @@ -113,7 +113,7 @@ def test_fallback_to_must_be_non_empty(self): def test_to_mcp_dict_matches_spec_sample(self): a = ToolAnnotation( requires=("collection_access",), - capabilities={"vision": False, "graph_index": True}, + capabilities={"vision": False, "web_access": True}, deprecated=False, deprecated_until=None, fallback_to=None, @@ -122,7 +122,7 @@ def test_to_mcp_dict_matches_spec_sample(self): # `capabilities`, JSON-friendly None. assert a.to_mcp_dict() == { "requires": ["collection_access"], - "capabilities": {"vision": False, "graph_index": True}, + "capabilities": {"vision": False, "web_access": True}, "deprecated": False, "deprecated_until": None, "fallback_to": None, @@ -134,10 +134,8 @@ def test_to_mcp_dict_matches_spec_sample(self): # --------------------------------------------------------------------------- -# The 12 tools that the D10.f spec scope annotates: 8 D10.c read -# primitives + 4 D10.d search primitives. Pre-D10 deprecated tools -# (search_collection / search_chat_files / web_read) are out of scope -# per §G boundary and intentionally NOT present. +# Built-in read/search/web tools must all carry annotations so any MCP +# client can make one consistent capability decision. _EXPECTED_TOOLS: frozenset[str] = frozenset( { # D10.c read primitives @@ -154,6 +152,7 @@ def test_to_mcp_dict_matches_spec_sample(self): "graph_search", "fulltext_search", "web_search", + "web_read", } ) @@ -186,15 +185,18 @@ def test_capability_keys_in_registry_are_known(self): for req in ann.requires: assert req in KNOWN_REQUIRES, f"tool {name!r} declares unknown requires entry {req!r}" - def test_graph_search_requires_graph_index(self): + def test_collection_search_tools_do_not_require_client_index_capabilities(self): + """Graph/fulltext indexes are collection state, not client + capabilities. Clients should not hide these tools just because + the host lacks a local graph/fulltext feature flag. + """ + ann = _annotations.get("graph_search") assert ann is not None - assert ann.capabilities.get("graph_index") is True - - def test_fulltext_search_requires_fulltext_index(self): + assert "graph_index" not in ann.capabilities ann = _annotations.get("fulltext_search") assert ann is not None - assert ann.capabilities.get("fulltext_index") is True + assert "fulltext_index" not in ann.capabilities def test_web_search_requires_web_access(self): ann = _annotations.get("web_search") @@ -202,6 +204,12 @@ def test_web_search_requires_web_access(self): assert ann.capabilities.get("web_access") is True assert "web_access" in ann.requires + def test_web_read_requires_web_access(self): + ann = _annotations.get("web_read") + assert ann is not None + assert ann.capabilities.get("web_access") is True + assert "web_access" in ann.requires + def test_read_document_marks_long_context(self): ann = _annotations.get("read_document") assert ann is not None @@ -281,14 +289,14 @@ def test_register_rejects_non_annotation_value(self): class TestFilterPseudocode: def test_needed_true_capability_excludes_lacking_client(self): - ann = ToolAnnotation(capabilities={"graph_index": True}) - usable, missing = is_usable(ann, {"graph_index": False}) + ann = ToolAnnotation(capabilities={"web_access": True}) + usable, missing = is_usable(ann, {"web_access": False}) assert usable is False - assert missing == ("graph_index",) + assert missing == ("web_access",) def test_needed_true_capability_admits_granting_client(self): - ann = ToolAnnotation(capabilities={"graph_index": True}) - usable, missing = is_usable(ann, {"graph_index": True}) + ann = ToolAnnotation(capabilities={"web_access": True}) + usable, missing = is_usable(ann, {"web_access": True}) assert usable is True assert missing == () @@ -309,10 +317,10 @@ def test_missing_client_capability_is_treated_as_False(self): assert missing == ("web_access",) def test_multiple_missing_caps_reported_sorted(self): - ann = ToolAnnotation(capabilities={"web_access": True, "graph_index": True, "vision": False}) + ann = ToolAnnotation(capabilities={"web_access": True, "long_context": True, "vision": False}) usable, missing = is_usable(ann, {"vision": True}) assert usable is False - assert missing == ("graph_index", "web_access") # sorted + assert missing == ("long_context", "web_access") # sorted # --------------------------------------------------------------------------- @@ -329,7 +337,7 @@ def _registry(self) -> dict[str, ToolAnnotation]: ), "graph_search": ToolAnnotation( requires=("collection_access",), - capabilities={"long_context": False, "graph_index": True}, + capabilities={"long_context": False}, ), "web_search": ToolAnnotation( requires=("web_access",), @@ -347,21 +355,21 @@ def _registry(self) -> dict[str, ToolAnnotation]: def test_air_gapped_client_excludes_web_with_reason(self): decisions = filter_tools( self._registry(), - {"long_context": True, "graph_index": True, "web_access": False}, + {"long_context": True, "web_access": False}, ) by_name = {d.tool_name: d for d in decisions} assert by_name["web_search"].usable is False assert by_name["web_search"].reason == "capability_required" assert by_name["web_search"].missing == ("web_access",) - def test_no_graph_index_client_excludes_graph_search(self): + def test_collection_index_state_does_not_hide_graph_search(self): decisions = filter_tools( self._registry(), - {"long_context": True, "graph_index": False, "web_access": True}, + {"long_context": True, "web_access": True}, ) by_name = {d.tool_name: d for d in decisions} - assert by_name["graph_search"].usable is False - assert by_name["graph_search"].missing == ("graph_index",) + assert by_name["graph_search"].usable is True + assert by_name["graph_search"].reason is None # vector_search has no needed=True caps so it must remain usable assert by_name["vector_search"].usable is True assert by_name["vector_search"].reason is None @@ -369,7 +377,7 @@ def test_no_graph_index_client_excludes_graph_search(self): def test_deprecated_tool_kept_with_reason_by_default(self): decisions = filter_tools( self._registry(), - {"long_context": True, "graph_index": True, "web_access": True}, + {"long_context": True, "web_access": True}, ) by_name = {d.tool_name: d for d in decisions} assert by_name["old_omnibus_search"].usable is True @@ -378,7 +386,7 @@ def test_deprecated_tool_kept_with_reason_by_default(self): def test_deprecated_tool_dropped_when_include_deprecated_false(self): decisions = filter_tools( self._registry(), - {"long_context": True, "graph_index": True, "web_access": True}, + {"long_context": True, "web_access": True}, include_deprecated=False, ) by_name = {d.tool_name: d for d in decisions} @@ -397,15 +405,16 @@ def test_filter_decision_is_immutable(self): def test_usable_tool_names_helper_returns_only_usable(self): names = usable_tool_names( self._registry(), - {"long_context": True, "graph_index": False, "web_access": False}, + {"long_context": True, "web_access": False}, ) # vector_search has no needed=True caps so it survives. + # graph_search is gated by collection state at execution time, + # not by client capability filtering. # old_omnibus_search is deprecated but still usable per default. - # graph_search and web_search are excluded by missing caps. - assert set(names) == {"vector_search", "old_omnibus_search"} + # web_search is excluded by missing web access. + assert set(names) == {"vector_search", "graph_search", "old_omnibus_search"} def test_required_capabilities_aggregates_only_true_keys(self): keys = required_capabilities(self._registry().values()) - # vision/long_context never appear as needed=True — only - # graph_index + web_access. Sorted output. - assert keys == ("graph_index", "web_access") + # collection index availability is not a client capability. + assert keys == ("web_access",) diff --git a/tests/unit_test/mcp/test_search_split.py b/tests/unit_test/mcp/test_search_split.py index d96ac88d4..8945bea22 100644 --- a/tests/unit_test/mcp/test_search_split.py +++ b/tests/unit_test/mcp/test_search_split.py @@ -12,20 +12,18 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Phase 9 D10.d (#96) — search primitives split contract tests. +"""Search primitive contract tests. Locks the public surface of the ``aperag.mcp.tools.search_*`` modules -introduced in D10.d: +after the omnibus search cutover: - 4 split tools (``vector_search`` / ``graph_search`` / ``fulltext_search`` / ``web_search``) exist with the §B canonical signatures. - They are registered under ``mcp_server`` via the ``@mcp_server.tool`` decorator chain at module import time (server.py re-export gate). -- ``search_collection`` and ``search_chat_files`` carry the - ``[DEPRECATED]`` banner in their docstrings per §B.5 / §H.1 / §H.2 — - but their implementation body is untouched (Forbidden per §G D10.d). -- ``search_collection`` keeps hitting ``/api/v2/collections/{id}/searches`` - during the deprecation window (no body changes). +- ``search_collection`` and ``search_chat_files`` stay removed. +- Collection-scoped search tools do not expose pagination cursor + placeholders until real search pagination exists. Caller-migration assertion semantics (§G hard gate #4): backward compatibility for existing ``mcp_server.web_search`` attribute access @@ -39,8 +37,6 @@ import inspect from pathlib import Path -import pytest - # Importing ``aperag.mcp.server`` triggers the bottom-of-module # registration block which loads ``aperag.mcp.tools.search_*`` and # triggers ``@mcp_server.tool`` decorators. The tool functions are @@ -101,9 +97,7 @@ def _kw_only_params(func) -> set[str]: def test_vector_search_signature_matches_b1_lock(): """``vector_search`` signature follows §B.1: ``collection_id`` + ``query`` positional, then a ``*,`` kw-only barrier with - ``top_k`` / ``similarity_threshold`` / ``rerank`` / ``cursor``. - The kw-only enforcement aligns with the D10.c precedent (per - `[D10 spec amendment]` msg=b9b7072a Drift #5 resolution). + ``top_k`` / ``similarity_threshold`` / ``rerank``. """ params = _params(vector_search) assert list(params)[:2] == ["collection_id", "query"] @@ -111,34 +105,29 @@ def test_vector_search_signature_matches_b1_lock(): "top_k", "similarity_threshold", "rerank", - "cursor", - }, "§B.1 requires kw-only `top_k / similarity_threshold / rerank / cursor`." + }, "§B.1 requires kw-only `top_k / similarity_threshold / rerank`." assert params["top_k"].default == 5 # similarity_threshold defaults to None to mean "use collection default". assert params["similarity_threshold"].default is None assert params["rerank"].default is True - # cursor placeholder per amendment msg=b9b7072a Drift #4 (c). - assert params["cursor"].default is None def test_graph_search_signature_matches_b2_lock(): - """``graph_search`` signature follows §B.2 + amendment msg=b9b7072a: - collection_id + query positional then kw-only top_k + cursor. + """``graph_search`` signature follows §B.2: + collection_id + query positional then kw-only top_k. The §B.2 spec mentions ``depth`` / ``entity_types`` as forward-looking knobs; first-cut keeps them out until the backend surface for graph traversal is wired (deferred to D10.d follow-up). """ params = _params(graph_search) assert list(params)[:2] == ["collection_id", "query"] - assert _kw_only_params(graph_search) == {"top_k", "cursor"}, "§B.2 requires kw-only `top_k / cursor`." + assert _kw_only_params(graph_search) == {"top_k"}, "§B.2 requires kw-only `top_k`." assert params["top_k"].default == 5 - assert params["cursor"].default is None def test_fulltext_search_signature_matches_b3_lock(): - """``fulltext_search`` signature follows §B.3 + amendment - msg=b9b7072a: collection_id + query positional then kw-only - top_k + keywords + rerank + cursor. + """``fulltext_search`` signature follows §B.3: collection_id + + query positional then kw-only top_k + keywords + rerank. """ params = _params(fulltext_search) assert list(params)[:2] == ["collection_id", "query"] @@ -146,12 +135,10 @@ def test_fulltext_search_signature_matches_b3_lock(): "top_k", "keywords", "rerank", - "cursor", - }, "§B.3 requires kw-only `top_k / keywords / rerank / cursor`." + }, "§B.3 requires kw-only `top_k / keywords / rerank`." assert params["top_k"].default == 5 assert params["keywords"].default is None assert params["rerank"].default is True - assert params["cursor"].default is None def test_web_search_signature_matches_b4_canonical(): @@ -180,126 +167,13 @@ def test_web_search_signature_matches_b4_canonical(): assert "max_results" not in params, "legacy `max_results` parameter must be gone" -# --- 2b. Cursor placeholder explicit-not-silent (§B / amendment Drift #4) --- - - -@pytest.mark.asyncio -@pytest.mark.parametrize( - "tool", - [vector_search, graph_search, fulltext_search], - ids=["vector_search", "graph_search", "fulltext_search"], -) -@pytest.mark.parametrize( - "bad_cursor", - ["garbage", "AAAA", "{}", "0"], - ids=["garbage", "base64_no_payload", "empty_json", "single_zero"], -) -async def test_collection_scoped_split_tools_reject_non_empty_cursor(tool, bad_cursor): - """Per `[D10 spec amendment]` msg=b9b7072a Drift #4 (c) + - architect sign-off msg=ebfcdabe + Weston blocker msg=177a1dd8: - any non-empty cursor must loud-fail with ``NotImplementedError`` - bearing a "search pagination is not yet implemented" message. - - The sign-off explicitly forbids reusing - ``CursorError("cursor_invalid", ...)`` for this case — the canonical - cursor codes describe wire-level malformed / expired / drifted - cursors, and the "feature not implemented yet" semantic must not - be camouflaged as a malformed-cursor error. - - ``cursor=None`` and ``cursor=""`` are covered by the - happy-path-passthrough test below; this case only exercises the - truthy-cursor loud-fail path. - """ - - with pytest.raises(NotImplementedError) as excinfo: - await tool("collection-id", "query", cursor=bad_cursor) - - message = str(excinfo.value) - assert "search pagination is not yet implemented" in message, ( - f"{tool.__name__} loud-fail message must clearly state that " - f"search pagination is not implemented (got {message!r})." - ) - assert tool.__name__ in message, ( - "Loud-fail message must identify the originating tool so logs can attribute the rejection." - ) - assert "search_not_paginated" in message, ( - "Loud-fail message must surface the deferred-pagination reason so clients understand they should not retry." - ) - - -@pytest.mark.asyncio -@pytest.mark.parametrize( - "tool", - [vector_search, graph_search, fulltext_search], - ids=["vector_search", "graph_search", "fulltext_search"], -) -@pytest.mark.parametrize( - "noop_cursor", - [None, ""], - ids=["none", "empty_string"], -) -async def test_collection_scoped_split_tools_treat_none_and_empty_cursor_as_first_page(tool, noop_cursor, monkeypatch): - """Weston blocker msg=177a1dd8 lock: ``None`` *and* ``""`` cursor - must both preserve the existing single-page ``top_k`` behavior - (``""`` is **not** a malformed cursor; it is the canonical - "no pagination requested" wire value). - - We stub ``get_api_key()`` and the outbound httpx call so the test - only validates that the cursor guard does **not** raise — it does - not exercise the backend search path itself. - """ - - import aperag.mcp.tools.search_fulltext as sft - import aperag.mcp.tools.search_graph as sg - import aperag.mcp.tools.search_vector as sv - - monkeypatch.setattr(sv, "get_api_key", lambda: "test-token") - monkeypatch.setattr(sg, "get_api_key", lambda: "test-token") - monkeypatch.setattr(sft, "get_api_key", lambda: "test-token") - - class _FakeResponse: - status_code = 200 - - def json(self): - return {"items": []} - - class _FakeAsyncClient: - def __init__(self, *args, **kwargs): - pass - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc, tb): - return False - - async def post(self, url, headers=None, json=None): - return _FakeResponse() - - import httpx - - monkeypatch.setattr(httpx, "AsyncClient", _FakeAsyncClient) - - # No exception expected — the cursor guard must not raise on - # ``None`` or ``""``. - result = await tool("collection-id", "query", cursor=noop_cursor) - assert isinstance(result, dict), ( - f"{tool.__name__} must return the SearchResult dict for None/empty cursor (got {type(result).__name__})." - ) - - -@pytest.mark.asyncio -async def test_web_search_does_not_carry_cursor_param(): - """``web_search`` is intentionally cursor-less — its provider - chain (Jina / DDG) is provider-bounded, not paginated, per §B.4. - The amendment thread (msg=b9b7072a Drift #4) only added cursor - placeholders to the 3 collection-scoped split tools. +def test_search_tools_do_not_carry_cursor_param(): + """Search pagination is not implemented, so search tools must not + advertise a cursor parameter in their MCP schemas. """ - params = _params(web_search) - assert "cursor" not in params, ( - "web_search must not carry a cursor parameter — its scope is provider-bounded per §B.4 (no pagination)." - ) + for tool in (vector_search, graph_search, fulltext_search, web_search): + assert "cursor" not in _params(tool), f"{tool.__name__} must not expose a cursor placeholder" # --- 3. Cutover removal of omnibus aliases (D10.h #100) --------------