From 031d524d0013f158690746698609a3a9a0873470 Mon Sep 17 00:00:00 2001 From: dajiaohuang Date: Sun, 30 Aug 2026 00:18:44 +0800 Subject: [PATCH] feat: bound MCP collection pagination --- .github/workflows/ci.yml | 2 +- README.md | 22 +- packages/mcp/README.md | 41 +- packages/mcp/evaluations/read_only.xml | 36 +- packages/mcp/src/sagasmith_dnd_mcp/server.py | 628 +++++++++++++++--- .../tests/test_bounded_catalog_pagination.py | 236 +++++++ .../mcp/tests/test_read_only_evaluations.py | 305 +++++++-- .../mcp/tests/test_statblock_import_mcp.py | 2 +- 8 files changed, 1097 insertions(+), 175 deletions(-) create mode 100644 packages/mcp/tests/test_bounded_catalog_pagination.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 84368eeb..bb822b7a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,7 +18,7 @@ jobs: include: - profile: locked-py311 python-version: "3.11" - core-ref: 302ce2b9cda575f927a3e4d7f13a310cd798a3d3 + core-ref: 612bfe7e5290eb5b23f2811baa83b8a28293b36e sync-args: --frozen - profile: compatibility-py312 python-version: "3.12" diff --git a/README.md b/README.md index 08931e48..ae2919e7 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,16 @@ does not rely on projection for security. opaque, owner-bound, expiring handle and does not change `tools/list` or confer permission. An expired or mismatched handle returns a structured recovery error. +Collection facades (`*_query`, `*_search`, event/history views, draft indexes, +content packs, NPC conversations, Skills, and exposure search) accept a bounded +filter plus `limit`/`top_k` of 1–100 and an opaque `cursor`. Successful list +results preserve the existing `result` and text fallback while adding `page` +and top-level `next_cursor`. A cursor is bound to its authorized collection and +filter; it is neither identity nor authority, and every continuation is +authorized again. Do not parse cursors. Restart at page one after an invalid or +expired cursor. `content_pack(include_package=true)` is the deliberate exception: +it requests one complete, finite import artifact rather than a catalog page. + ## Install and run Python 3.11 or newer is required. @@ -283,10 +293,14 @@ uv run --package sagasmith-dnd-mcp pytest \ ``` The repository contains ten independent, deterministic, read-only MCP Builder -evaluations in `packages/mcp/evaluations/read_only.xml`; the test fixture solves -and verifies every answer without a paid model or external service. Write tests -separately cover authorization, idempotency, stale revisions, concurrency, -cancellation, restart recovery, structured errors, media, and transport parity. +evaluations in `packages/mcp/evaluations/read_only.xml`. Each question starts a +fresh traversal of a six-campaign, 30-actor fixture, follows multiple campaign +and roster continuation pages, inspects actor details, resolves the system +catalog, and computes its own answer. The test requires at least 35 public-tool +calls and seven cursor continuations per question and verifies every answer +without a paid model or external service. Write tests separately cover +authorization, idempotency, stale revisions, concurrency, cancellation, restart +recovery, structured errors, media, and transport parity. ## Upgrade and rollback diff --git a/packages/mcp/README.md b/packages/mcp/README.md index 81da7c23..7f7400ea 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -88,6 +88,34 @@ metadata are evaluated per request. Cross-call state is explicit: Handles are names, never capabilities. Every use is independently authorized. +### Bounded collection contract + +All potentially growing list/search/query surfaces use the same continuation +contract. Supply a text `query`, `limit` (or `top_k` for ranked search) from 1 +through 100, and the previous opaque `cursor`. The response retains the +compatible `result` plus concise text and adds: + +```json +{ + "page": { + "limit": 50, + "returned": 50, + "has_more": true, + "next_cursor": "opaque" + }, + "next_cursor": "opaque" +} +``` + +The cursor is scoped to the authorized collection and normalized filter. It +must not be parsed, stored as a credential, or reused with another campaign, +principal, view, branch, or query. Continuations rerun authorization. On an +invalid/expired cursor, restart without a cursor. Older facade clients may place +the cursor, query, limit, or offset inside `payload`; new clients should use the +top-level fields. A materialized catalog page also reports `total_count` when it +is available without reading unbounded history. `content_pack(include_package=true)` +explicitly retrieves one complete finite import artifact and is not a catalog list. + ### Stable catalog and bounded Host projection The current contract test locks 77 public tools. `tools/list` returns them in @@ -324,11 +352,14 @@ uv run --package sagasmith-dnd-mcp pytest \ ``` `evaluations/read_only.xml` contains ten independent, complex, deterministic, -read-only MCP Builder questions. `test_read_only_evaluations.py` creates a fixed -fixture, solves every question through read-only public tools, and verifies the -answers without a paid model or external service. Write-side tests separately -cover idempotency, authorization isolation, stale revisions, concurrency, -cancellation, restart recovery, and structured errors. +read-only MCP Builder questions. For every question, +`test_read_only_evaluations.py` independently traverses multiple pages of six +campaigns and 30 nested actors, reads actor detail records, resolves system +metadata, and computes the answer. The test rejects a nominal ten-question file +unless each solver performs at least 35 public-tool calls and seven cursor +continuations. It uses no paid model or external service. Write-side tests +separately cover idempotency, authorization isolation, stale revisions, +concurrency, cancellation, restart recovery, and structured errors. The opt-in real-provider corpus regression is documented in [`docs/FULL_AGENT_REGRESSION.md`](docs/FULL_AGENT_REGRESSION.md). It is not part diff --git a/packages/mcp/evaluations/read_only.xml b/packages/mcp/evaluations/read_only.xml index 8dd67bdc..7bfaaa31 100644 --- a/packages/mcp/evaluations/read_only.xml +++ b/packages/mcp/evaluations/read_only.xml @@ -1,43 +1,43 @@ - Across every active campaign in the deterministic evaluation fixture, inspect each campaign and its actor roster. Which campaign has the largest roster? - Alpha Expedition + Traverse the complete campaign catalog through pagination, then page through every campaign roster. Compare NPC counts across campaigns; which campaign has the largest NPC roster? + Ivory Archive - Explore all active campaigns and all of their actors. Considering only player characters, which actor name is alphabetically last? - Zephyr + Explore every paginated campaign and roster, separate monsters from PCs and NPCs, and compare monster totals by campaign. Which campaign contains the most monsters? + Violet March - Find the sole actor classified as a monster, then resolve its campaign. What is that campaign's name? - Beta Citadel + After paginating across all campaigns and their complete rosters, keep only campaigns with exactly two player characters. Of those, which has the larger total roster? + Cobalt Labyrinth - Enumerate all active campaigns and their complete actor rosters. How many actors are classified as NPCs in total? - 4 + Walk all campaign pages and all nested roster pages. Considering only actors classified as player characters, what is the alphabetically last actor name? + Zephyr - After traversing every active campaign and its actor roster, how many campaign-bound actors exist altogether? - 7 + Discover all campaigns, page through their rosters, and inspect the detailed actor records to find the keeper whose summary mentions the sealed lens. What is that actor's campaign slug? + amber-observatory - Compare role composition across the active campaigns. Which campaign has the same number of player characters and NPCs? - Beta Citadel + Locate the campaign whose description says tidal bells guide navigation, then page through its roster and sort only its NPCs. Which NPC name comes first alphabetically? + Quill - Read the detailed records for every actor and locate the summary that mentions a sealed observatory. What is that actor's name? - Nyx + Paginate every campaign and roster, exclude every campaign that has any monsters, and compare the remaining NPC-to-PC ratios. Which campaign has the highest ratio? + Ivory Archive - Resolve the roster belonging to Beta Citadel and count the distinct actor classifications represented there. What is the count? + Find the campaign with the largest total roster after traversing all campaign and character pages. How many distinct actor classifications appear in that roster? 3 - Resolve each campaign's system identifier through the server's supported-system catalog. What display name is shared by both fixture campaigns? + Sort the fully paginated campaign catalog by name, compare the alphabetically first and last campaigns, and resolve their shared system identifier through the supported-system catalog. What display name does it have? Dungeons & Dragons 5e - Sort the active campaigns by their human-readable names, select the first, and report its canonical slug. - alpha-expedition + Across every campaign and complete paginated roster, find the campaign where the monster count equals the combined PC and NPC count. What is that campaign's canonical slug? + violet-march diff --git a/packages/mcp/src/sagasmith_dnd_mcp/server.py b/packages/mcp/src/sagasmith_dnd_mcp/server.py index 1c3dcd64..64741e48 100644 --- a/packages/mcp/src/sagasmith_dnd_mcp/server.py +++ b/packages/mcp/src/sagasmith_dnd_mcp/server.py @@ -3,6 +3,8 @@ from __future__ import annotations import asyncio +import base64 +import binascii import hashlib import importlib import inspect @@ -815,17 +817,194 @@ def _pending_result_ruling_kind( return nested_ruling_kind(result, fallback=fallback) -def _facade_result(action: str, result: Any) -> dict[str, Any]: +def _facade_result( + action: str, + result: Any, + *, + page: dict[str, Any] | None = None, +) -> dict[str, Any]: """Preserve a nested domain ruling's ownership at the public facade.""" status = result.get("status", "ok") if isinstance(result, dict) else "ok" response = {"status": status, "action": action, "result": result} + if page is not None: + response["page"] = page + response["next_cursor"] = page.get("next_cursor") if status == "pending_ruling" and isinstance(result, dict): ruling_kind = _pending_result_ruling_kind(result) response.update(_ruling_resolution_for_kind(ruling_kind)) return response +def _bounded_page( + values: list[Any], + *, + scope: str, + query: str = "", + limit: int = 50, + cursor: str | None = None, + offset: int = 0, +) -> tuple[list[Any], dict[str, Any]]: + """Filter and page one already-authorized collection with an opaque cursor. + + The cursor is a continuation name rather than a capability. It is bound to + the tool/view/filter scope so a client cannot accidentally reuse it for a + different authorized collection. Authorization is still rechecked before + this helper receives any values. + """ + + if isinstance(limit, bool) or not isinstance(limit, int) or not 1 <= limit <= 100: + raise ValueError("limit must be an integer between 1 and 100") + if isinstance(offset, bool) or not isinstance(offset, int) or not 0 <= offset <= 100_000: + raise ValueError("offset must be an integer between 0 and 100000") + normalized_query = " ".join(str(query or "").split()).casefold() + fingerprint = json_sha256({"scope": scope, "query": normalized_query})[:24] + if cursor: + if offset: + raise ValueError("cursor and offset are mutually exclusive") + try: + padded = str(cursor) + "=" * (-len(str(cursor)) % 4) + cursor_payload = json.loads(base64.urlsafe_b64decode(padded).decode("utf-8")) + if ( + not isinstance(cursor_payload, dict) + or cursor_payload.get("v") != 1 + or cursor_payload.get("f") != fingerprint + or isinstance(cursor_payload.get("o"), bool) + or not isinstance(cursor_payload.get("o"), int) + or not 0 <= int(cursor_payload["o"]) <= 100_000 + ): + raise ValueError + offset = int(cursor_payload["o"]) + except ( + binascii.Error, + UnicodeDecodeError, + ValueError, + TypeError, + json.JSONDecodeError, + ) as exc: + raise ValueError( + "cursor is invalid for this query; restart from the first page" + ) from exc + + filtered = list(values) + if normalized_query: + filtered = [ + value + for value in filtered + if normalized_query + in json.dumps(value, ensure_ascii=False, sort_keys=True, default=str).casefold() + ] + page_values = filtered[offset : offset + limit] + next_offset = offset + len(page_values) + has_more = next_offset < len(filtered) + next_cursor = None + if has_more: + raw = json.dumps( + {"v": 1, "f": fingerprint, "o": next_offset}, + separators=(",", ":"), + sort_keys=True, + ).encode("utf-8") + next_cursor = base64.urlsafe_b64encode(raw).decode("ascii").rstrip("=") + return page_values, { + "limit": limit, + "returned": len(page_values), + "has_more": has_more, + "next_cursor": next_cursor, + "total_count": len(filtered), + } + + +def _page_limit(value: Any) -> int: + if isinstance(value, bool) or not isinstance(value, int) or not 1 <= value <= 100: + raise ValueError("limit must be an integer between 1 and 100") + return value + + +def _cursor_offset( + *, + scope: str, + query: str = "", + cursor: str | None = None, + offset: int = 0, +) -> tuple[str, int]: + """Decode one scope-bound continuation cursor before an authority query.""" + + if isinstance(offset, bool) or not isinstance(offset, int) or not 0 <= offset <= 100_000: + raise ValueError("offset must be an integer between 0 and 100000") + normalized_query = " ".join(str(query or "").split()).casefold() + fingerprint = json_sha256({"scope": scope, "query": normalized_query})[:24] + if not cursor: + return fingerprint, offset + if offset: + raise ValueError("cursor and offset are mutually exclusive") + try: + padded = str(cursor) + "=" * (-len(str(cursor)) % 4) + payload = json.loads(base64.urlsafe_b64decode(padded).decode("utf-8")) + if ( + not isinstance(payload, dict) + or payload.get("v") != 1 + or payload.get("f") != fingerprint + or isinstance(payload.get("o"), bool) + or not isinstance(payload.get("o"), int) + or not 0 <= int(payload["o"]) <= 100_000 + ): + raise ValueError + return fingerprint, int(payload["o"]) + except ( + binascii.Error, + UnicodeDecodeError, + ValueError, + TypeError, + json.JSONDecodeError, + ) as exc: + raise ValueError("cursor is invalid for this query; restart from the first page") from exc + + +def _encode_cursor(fingerprint: str, offset: int) -> str: + raw = json.dumps( + {"v": 1, "f": fingerprint, "o": offset}, + separators=(",", ":"), + sort_keys=True, + ).encode("utf-8") + return base64.urlsafe_b64encode(raw).decode("ascii").rstrip("=") + + +def _authority_page( + values: list[Any], + *, + fingerprint: str, + offset: int, + limit: int, + query: str = "", + chronological_tail: bool = False, +) -> tuple[list[Any], dict[str, Any]]: + """Build page metadata for a limit+1 authority query. + + EventService returns each newest window in chronological order, so its + look-ahead record is at the front. RevisionService is newest-first, so its + look-ahead record is at the end. + """ + + limit = _page_limit(limit) + has_more = len(values) > limit + selected = values[-limit:] if chronological_tail else values[:limit] + normalized_query = " ".join(str(query or "").split()).casefold() + if normalized_query: + selected = [ + value + for value in selected + if normalized_query + in json.dumps(value, ensure_ascii=False, sort_keys=True, default=str).casefold() + ] + next_cursor = _encode_cursor(fingerprint, offset + limit) if has_more else None + return selected, { + "limit": limit, + "returned": len(selected), + "has_more": has_more, + "next_cursor": next_cursor, + } + + def _needs_ruling_kind( error: NeedsRulingError | RuleEventRulingRequiredError, ) -> str: @@ -29320,6 +29499,7 @@ def event_list( branch_id: str | None = None, actor_id: str | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + offset: int = 0, ) -> list[dict[str, Any]]: membership = access.require_campaign(campaign_id, principal_id) resolved_branch_id = readable_branch(campaign_id, branch_id, principal_id) @@ -29337,6 +29517,7 @@ def event_list( audience=audience, actor_id=actor_id, limit=limit, + offset=offset, branch_id=resolved_branch_id, ) return [asdict(item) for item in values] @@ -29548,10 +29729,13 @@ def state_history( campaign_id: str, limit: int = 100, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + offset: int = 0, ) -> list[dict[str, Any]]: """List audited reversible campaign and character mutations.""" access.require_campaign(campaign_id, principal_id, roles=CAMPAIGN_DM_ROLES) - return [asdict(item) for item in revisions.history(campaign_id, limit=limit)] + return [ + asdict(item) for item in revisions.history(campaign_id, limit=limit, offset=offset) + ] def state_undo( campaign_id: str, @@ -31266,6 +31450,9 @@ def npc_conversation( action: Literal["open", "list", "get", "ingest", "publish", "close", "abort"], payload: dict[str, Any], principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Run the complete public NPC conversation workflow through one facade.""" @@ -31289,7 +31476,22 @@ def npc_conversation( principal_id=principal_id, ) if action == "list": - return npc_conversation_list_impl(campaign_id, principal_id) + listing = npc_conversation_list_impl(campaign_id, principal_id) + conversations, page = _bounded_page( + list(listing.get("conversations") or []), + scope=f"npc_conversation:list:{campaign_id}:{principal_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return { + **listing, + "count": len(conversations), + "conversations": conversations, + "page": page, + "next_cursor": page["next_cursor"], + } conversation_id = str(data["conversation_id"]) if action == "get": return npc_conversation_status_impl(campaign_id, conversation_id, principal_id) @@ -32958,7 +33160,8 @@ def module_search( top_k: int = 8, module_ids: list[str] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, - ) -> list[dict[str, Any]]: + cursor: Annotated[str | None, Field(max_length=1024)] = None, + ) -> dict[str, Any]: """Search adventure content, optionally scoped to exact active module revisions.""" membership = access.require_campaign(campaign_id, principal_id) embedder, vectors = storage.dense_components() @@ -32966,18 +33169,30 @@ def module_search( campaign_id=campaign_id, query=query, query_hints=DND5E_QUERY_HINTS, - top_k=top_k, + top_k=100, module_ids=module_ids, embedder=embedder, vector_store=vectors, ) if membership.role in CAMPAIGN_DM_ROLES: - return [asdict(hit) for hit in hits] - return [ - asdict(hit) - for hit in hits - if hit.metadata.get("visibility", "restricted") in PLAYER_MODULE_VISIBILITY_SCOPES - ] + values = [asdict(hit) for hit in hits] + else: + values = [ + asdict(hit) + for hit in hits + if hit.metadata.get("visibility", "restricted") + in PLAYER_MODULE_VISIBILITY_SCOPES + ] + values, page = _bounded_page( + values, + scope=( + f"module_search:{campaign_id}:{principal_id}:{query}:" + f"{','.join(sorted(module_ids or []))}" + ), + limit=top_k, + cursor=cursor, + ) + return _facade_result("search", values, page=page) def campaign_rule_source_ids(campaign_id: str) -> set[str]: """Resolve the indexed sources visible to this campaign and branch.""" @@ -33045,7 +33260,8 @@ def rule_search( ] = None, top_k: int = 8, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, - ) -> list[dict[str, Any]]: + cursor: Annotated[str | None, Field(max_length=1024)] = None, + ) -> dict[str, Any]: """Search rules visible to the campaign; first lookup needs only id and query.""" access.require_campaign(campaign_id, principal_id) if not str(query or "").strip(): @@ -33145,7 +33361,17 @@ def optional_text_list(field_name: str) -> list[str] | None: + "; omit source_keys unless exact source evidence supplies one" ) if not allowed_source_ids: - return [] + return _facade_result( + "search", + [], + page={ + "limit": top_k, + "returned": 0, + "has_more": False, + "next_cursor": None, + "total_count": 0, + }, + ) embedder, vectors = storage.dense_components() hits = rules.search( system_id=DND5E.id, @@ -33156,7 +33382,7 @@ def optional_text_list(field_name: str) -> list[str] | None: publications=publications, source_ids=sorted(allowed_source_ids), source_keys=source_keys, - top_k=top_k, + top_k=100, embedder=embedder, vector_store=vectors, ) @@ -33169,7 +33395,16 @@ def optional_text_list(field_name: str) -> list[str] | None: <= page <= int(dict(item.get("metadata") or {}).get("page_end") or 0) ] - return values + values, pagination = _bounded_page( + values, + scope=( + f"rule_search:{campaign_id}:{principal_id}:{query}:" + f"{json_sha256(filter_data)}" + ), + limit=top_k, + cursor=cursor, + ) + return _facade_result("search", values, page=pagination) @public_tool() def rule_expand( @@ -41696,8 +41931,13 @@ def optional_datetime(value: Any, name: str) -> datetime | None: except ValueError as exc: raise ValueError(f"payload.{name} must be ISO-8601") from exc - def facade_result(action: str, result: Any) -> dict[str, Any]: - return _facade_result(action, result) + def facade_result( + action: str, + result: Any, + *, + page: dict[str, Any] | None = None, + ) -> dict[str, Any]: + return _facade_result(action, result, page=page) def facade_render_result(rendered: Any) -> CallToolResult: """Preserve native image content while satisfying a facade's structured output.""" @@ -41988,6 +42228,9 @@ def module_query( ] = "list", payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read module cards, indexes, one scene, or current scoped progress.""" data = facade_payload(payload) @@ -42046,6 +42289,16 @@ def module_query( data.get("module_id"), principal_id, ) + if isinstance(result, list): + result, page = _bounded_page( + result, + scope=f"module_query:{campaign_id}:{view}:{principal_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) def _content_pack_actor_presets( @@ -42435,6 +42688,9 @@ def rulebook_draft( principal_id: Annotated[str, Field(title="Principal")] = LOCAL_SYSTEM_PRINCIPAL_ID, expected_revision: Annotated[int | None, Field(title="Revision")] = None, idempotency_key: Annotated[str | None, Field(title="Request Key")] = None, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Create, inspect, edit, and finalize one source-bound rulebook draft.""" @@ -42444,16 +42700,36 @@ def rulebook_draft( if data.get("job_id"): job = import_job_get(campaign_id, str(data["job_id"]), principal_id) source = rules.source(str(job["source_id"])) if job.get("source_id") else None + candidates, page = _bounded_page( + list(job.get("candidates") or []), + scope=( + f"rulebook_draft:get:{campaign_id}:{principal_id}:" + f"{str(data['job_id'])}:candidates" + ), + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + job = {**job, "candidates": candidates} result = { "job": job, - "candidates": list(job.get("candidates") or []), + "candidates": candidates, "inspection": deepcopy(job.get("inspection")), "source_id": job.get("source_id"), "source": source, } else: - result = {"jobs": import_job_list(campaign_id, "rulebook", principal_id)} - return facade_result(action, result) + jobs, page = _bounded_page( + import_job_list(campaign_id, "rulebook", principal_id), + scope=f"rulebook_draft:get:{campaign_id}:{principal_id}:jobs", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + result = {"jobs": jobs} + return facade_result(action, result, page=page) if action == "start": data = facade_payload(payload) @@ -42551,16 +42827,15 @@ def rulebook_draft( and isinstance(item.get("page_end"), int) and int(item["page_start"]) <= int(page_number) <= int(item["page_end"]) ] - query = str(data.get("query") or "").strip().casefold() - if query: - chunks = [ - item for item in chunks if query in str(item.get("content") or "").casefold() - ] - offset = int(data.get("offset", 0)) - limit = int(data.get("limit", 50)) - if offset < 0 or not 1 <= limit <= 200: - raise ValueError("payload offset/limit is outside the supported range") - return facade_result(action, chunks[offset : offset + limit]) + chunks, page = _bounded_page( + chunks, + scope=f"rulebook_draft:evidence:{campaign_id}:{principal_id}:{job_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(action, chunks, page=page) if action == "edit": operation = str(required(data, "operation")) @@ -42884,6 +43159,9 @@ def module_draft( principal_id: Annotated[str, Field(title="Principal")] = LOCAL_SYSTEM_PRINCIPAL_ID, expected_revision: Annotated[int | None, Field(title="Revision")] = None, idempotency_key: Annotated[str | None, Field(title="Request Key")] = None, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Create, inspect, edit, and finalize one source-bound module draft.""" @@ -42899,15 +43177,24 @@ def module_draft( jobs = [import_job_get(campaign_id, str(data["job_id"]), principal_id)] else: access.require_campaign(campaign_id, principal_id, roles=CAMPAIGN_DM_ROLES) + jobs, page = _bounded_page( + [ + module_draft_handle_view(item) + for item in import_jobs.list(campaign_id, kind="module") + ], + scope=f"module_draft:get:{campaign_id}:{principal_id}:jobs", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) return facade_result( action, { "order": "newest_first", - "jobs": [ - module_draft_handle_view(item) - for item in import_jobs.list(campaign_id, kind="module") - ], + "jobs": jobs, }, + page=page, ) if view == "package": job = require_import_job(campaign_id, str(data["job_id"]), "module") @@ -43043,25 +43330,18 @@ def chunk_evidence_receipt( job.module_id, scene_id=(str(data["scene_id"]) if data.get("scene_id") else None), ) - query = str(data.get("query") or "").strip().casefold() - if query: - chunks = [ - item - for item in chunks - if query - in "\n".join( - [ - *[str(value) for value in item.get("heading_path", [])], - str(item.get("content") or ""), - ] - ).casefold() - ] - limit = data.get("limit", 100) - if isinstance(limit, bool) or not isinstance(limit, int) or not 1 <= limit <= 500: - raise ValueError("payload.limit must be an integer between 1 and 500") + chunks, page = _bounded_page( + chunks, + scope=f"module_draft:evidence:{campaign_id}:{principal_id}:{job_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) return facade_result( action, - [chunk_evidence_receipt(item) for item in chunks[:limit]], + [chunk_evidence_receipt(item) for item in chunks], + page=page, ) if evidence_kind != "page": raise ValueError("payload.kind must be page or chunks") @@ -43667,6 +43947,9 @@ def content_pack( principal_id: Annotated[str, Field(title="Principal")] = LOCAL_SYSTEM_PRINCIPAL_ID, expected_revision: Annotated[int | None, Field(title="Revision")] = None, idempotency_key: Annotated[str | None, Field(title="Request Key")] = None, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Inspect and manage finalized core-rules, addon, module, and preset Packs.""" @@ -43724,6 +44007,18 @@ def content_pack( if data.get("include_package") is True else _content_pack_actor_preset_list(data, principal_id) ) + if isinstance(result, list): + result, page = _bounded_page( + result, + scope=f"content_pack:list:{campaign_id}:{kind}:{principal_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(action, result, page=page) + # include_package=True intentionally returns one complete, bounded + # preset artifact for import and is not a catalog listing. return facade_result(action, result) if action == "get": @@ -44106,6 +44401,7 @@ def character_query( query: Annotated[str, Field(max_length=200)] = "", limit: Annotated[int, Field(ge=1, le=100)] = 50, offset: Annotated[int, Field(ge=0, le=100_000)] = 0, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read characters with bounded filtering/paging or inspect an allowlisted document.""" data = facade_payload(payload) @@ -44306,20 +44602,22 @@ def character_query( } else: result = character_list(data.get("campaign_id"), principal_id) - needle = query.strip().casefold() - if needle: - result = [ - item - for item in result - if needle - in ( - f"{item.get('name', '')} {item.get('summary', '')} " - f"{item.get('character_type', '')}" - ).casefold() - ] result = sorted( result, key=lambda item: (str(item.get("name", "")), str(item.get("id", ""))) - )[offset : offset + limit] + ) + if view in {"list", "library", "catalog"} and isinstance(result, list): + result, page = _bounded_page( + result, + scope=( + f"character_query:{view}:{principal_id}:" + f"{str(data.get('campaign_id') or '')}" + ), + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=offset or data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) def dependent_actor_source_text( @@ -46343,6 +46641,7 @@ def campaign_query( query: Annotated[str, Field(max_length=200)] = "", limit: Annotated[int, Field(ge=1, le=100)] = 50, offset: Annotated[int, Field(ge=0, le=100_000)] = 0, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read bounded campaign pages, party state, or one complete resume bundle.""" data = facade_payload(payload) @@ -46411,20 +46710,18 @@ def campaign_query( result = party_show(required(data, "campaign_id"), principal_id) else: result = campaign_list(data.get("status"), principal_id) - needle = query.strip().casefold() - if needle: - result = [ - item - for item in result - if needle - in ( - f"{item.get('name', '')} {item.get('slug', '')} " - f"{item.get('description', '')}" - ).casefold() - ] result = sorted( result, key=lambda item: (str(item.get("name", "")), str(item.get("id", ""))) - )[offset : offset + limit] + ) + result, page = _bounded_page( + result, + scope=f"campaign_query:list:{principal_id}:{str(data.get('status') or '')}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=offset or data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) @public_tool() @@ -47164,6 +47461,9 @@ def campaign_event( payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, idempotency_key: str | None = None, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Append an auditable campaign event or retrieve its branch-visible event log.""" data = facade_payload(payload) @@ -47183,13 +47483,35 @@ def campaign_event( idempotency_key, ) else: + effective_query = query or str(data.get("query") or "") + page_limit = _page_limit(data.get("limit", limit)) + page_scope = ( + f"campaign_event:list:{campaign_id}:{principal_id}:" + f"{str(data.get('branch_id') or '')}:{str(data.get('actor_id') or '')}" + ) + fingerprint, page_offset = _cursor_offset( + scope=page_scope, + query=effective_query, + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) result = event_list( campaign_id, - data.get("limit", 50), + page_limit + 1, data.get("branch_id"), data.get("actor_id"), principal_id, + page_offset, + ) + result, page = _authority_page( + result, + fingerprint=fingerprint, + offset=page_offset, + limit=page_limit, + query=effective_query, + chronological_tail=True, ) + return facade_result(action, result, page=page) return facade_result(action, result) @public_tool() @@ -47198,6 +47520,9 @@ def memory_query( view: Literal["list", "search", "diagnostics"] = "list", payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read objective campaign memory; actor knowledge remains a separate subjective store.""" if view == "diagnostics": @@ -47217,11 +47542,12 @@ def memory_query( ), ) data = facade_payload(payload) + effective_query = query or str(data.get("query") or "") result = ( memory_search( campaign_id, - required(data, "query"), - data.get("limit", 8), + required(data, "query") if not query else query, + 100, data.get("branch_id"), principal_id, data.get("include_inactive", False), @@ -47235,6 +47561,20 @@ def memory_query( data.get("include_inactive", False), ) ) + if isinstance(result, list): + result, page = _bounded_page( + result, + scope=( + f"memory_query:{campaign_id}:{view}:{principal_id}:" + f"{str(data.get('branch_id') or '')}:{bool(data.get('include_inactive'))}:" + f"{json_sha256(effective_query)}" + ), + query="" if view == "search" else effective_query, + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) @public_tool() @@ -47416,22 +47756,37 @@ def actor_knowledge_query( view: Literal["list", "search"] = "list", payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read only one actor's branch-scoped, subjective knowledge.""" data = facade_payload(payload) + effective_query = query or str(data.get("query") or "") result = ( actor_knowledge_search( campaign_id, actor_id, - required(data, "query"), + required(data, "query") if not query else query, data.get("branch_id"), - data.get("limit", 8), + 100, principal_id, ) if view == "search" else actor_knowledge_list(campaign_id, actor_id, data.get("branch_id"), principal_id) ) - return facade_result(view, result) + result, page = _bounded_page( + result, + scope=( + f"actor_knowledge_query:{campaign_id}:{actor_id}:{view}:{principal_id}:" + f"{str(data.get('branch_id') or '')}:{json_sha256(effective_query)}" + ), + query="" if view == "search" else effective_query, + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(view, result, page=page) @public_tool() def actor_knowledge_change( @@ -47480,6 +47835,9 @@ def branch_query( view: Literal["list", "compare"] = "list", payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """List branches or compare two branch heads without changing checkout state.""" data = facade_payload(payload) @@ -47493,6 +47851,16 @@ def branch_query( if view == "compare" else branch_list(campaign_id, principal_id) ) + if view == "list": + result, page = _bounded_page( + result, + scope=f"branch_query:{campaign_id}:{principal_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) @public_tool() @@ -47548,6 +47916,9 @@ def snapshot_query( view: Literal["list", "verify", "lineage", "recap", "core"] = "list", payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read snapshot history, integrity, lineage, or a regenerated recap.""" data = facade_payload(payload) @@ -47561,6 +47932,16 @@ def snapshot_query( result = snapshot_regenerate_recap(campaign_id, required(data, "slot"), principal_id) else: result = snapshot_core_lock(campaign_id, required(data, "slot"), principal_id) + if isinstance(result, list): + result, page = _bounded_page( + result, + scope=f"snapshot_query:{campaign_id}:{view}:{principal_id}", + query=query or str(data.get("query") or ""), + limit=data.get("limit", limit), + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + return facade_result(view, result, page=page) return facade_result(view, result) @public_tool() @@ -47570,11 +47951,23 @@ def state_revision( payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, idempotency_key: str | None = None, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read revision history or perform guarded undo/redo.""" data = facade_payload(payload) if action == "history": - result = state_history(campaign_id, data.get("limit", 100), principal_id) + effective_query = query or str(data.get("query") or "") + page_limit = _page_limit(data.get("limit", limit)) + page_scope = f"state_revision:history:{campaign_id}:{principal_id}" + fingerprint, page_offset = _cursor_offset( + scope=page_scope, + query=effective_query, + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + result = state_history(campaign_id, page_limit + 1, principal_id, page_offset) elif action == "receipt": receipt_key = str(required(data, "idempotency_key")).strip() if not receipt_key: @@ -47593,6 +47986,15 @@ def state_revision( result = state_redo( campaign_id, principal_id, data.get("expected_history_sequence"), idempotency_key ) + if action == "history": + result, page = _authority_page( + result, + fingerprint=fingerprint, + offset=page_offset, + limit=page_limit, + query=effective_query, + ) + return facade_result(action, result, page=page) return facade_result(action, result) @public_tool() @@ -47609,6 +48011,9 @@ def combat_query( actor_id: str | None = None, payload: dict[str, Any] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, + query: Annotated[str, Field(max_length=200)] = "", + limit: Annotated[int, Field(ge=1, le=100)] = 50, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Read combat state, render a safe snapshot, or inspect DM-only transactions.""" data = facade_payload(payload) @@ -47634,7 +48039,16 @@ def combat_query( ) ) elif view == "transaction_history": - result = state_history(campaign_id, data.get("limit", 100), principal_id) + effective_query = query or str(data.get("query") or "") + page_limit = _page_limit(data.get("limit", limit)) + page_scope = f"combat_query:{campaign_id}:{view}:{principal_id}" + fingerprint, page_offset = _cursor_offset( + scope=page_scope, + query=effective_query, + cursor=cursor or data.get("cursor"), + offset=data.get("offset", 0), + ) + result = state_history(campaign_id, page_limit + 1, principal_id, page_offset) else: receipt_key = str(required(data, "idempotency_key")).strip() if not receipt_key: @@ -47645,6 +48059,15 @@ def combat_query( data.get("branch_id"), principal_id, ) + if view == "transaction_history": + result, page = _authority_page( + result, + fingerprint=fingerprint, + offset=page_offset, + limit=page_limit, + query=effective_query, + ) + return facade_result(view, result, page=page) return facade_result(view, result) @public_tool() @@ -48094,6 +48517,7 @@ def skill_query( query: str | None = None, max_chars: int = 12_000, limit: int = 8, + cursor: Annotated[str | None, Field(max_length=1024)] = None, ) -> dict[str, Any]: """Discover or read bounded installed workflow guidance.""" if action == "outline": @@ -48113,7 +48537,7 @@ def skill_query( kind=kind, identifier=identifier, query=required({"query": query}, "query"), - limit=limit, + limit=100, ) elif kind == "skill": result = ( @@ -48127,6 +48551,18 @@ def skill_query( if action == "list" else skill_asset_read(required({"identifier": identifier}, "identifier")) ) + if isinstance(result, list): + result, page = _bounded_page( + result, + scope=( + f"skill_query:{kind}:{action}:{str(identifier or source or '')}:" + f"{json_sha256(str(query or ''))}" + ), + query="" if action == "search" else str(query or ""), + limit=limit, + cursor=cursor, + ) + return facade_result(action, result, page=page) return facade_result(action, result) @public_tool() @@ -48163,6 +48599,7 @@ async def exposure( query: Annotated[str, Field(max_length=200)] = "", limit: Annotated[int, Field(ge=1, le=50)] = 20, offset: Annotated[int, Field(ge=0, le=10_000)] = 0, + cursor: Annotated[str | None, Field(max_length=1024)] = None, add_tool_ids: list[str] | None = None, remove_tool_ids: list[str] | None = None, principal_id: str = LOCAL_SYSTEM_PRINCIPAL_ID, @@ -48272,17 +48709,22 @@ async def exposure( } ) matches = sorted(matches, key=lambda item: str(item["tool_id"])) - page = matches[offset : offset + limit] + page_items, page = _bounded_page( + matches, + scope=( + f"exposure:search:{current.id}:{current.revision}:" + f"{' '.join(sorted(terms))}" + ), + limit=limit, + cursor=cursor, + offset=offset, + ) result = { **exposures.status(current), "query_semantics": "all_terms_match_one_tool", - "matches": page, - "page": { - "offset": offset, - "limit": limit, - "returned": len(page), - "has_more": offset + limit < len(matches), - }, + "matches": page_items, + "page": page, + "next_cursor": page["next_cursor"], } if terms and not matches: result["next"] = ( @@ -48326,6 +48768,8 @@ async def exposure( parameter_schema.update({"minimum": 0, "maximum": 100_000}) elif parameter_name in {"query", "name", "label", "identifier"}: parameter_schema.setdefault("maxLength", 256) + elif parameter_name == "cursor": + parameter_schema.setdefault("maxLength", 1024) elif parameter_name.endswith("_id") or parameter_name in { "action", "kind", diff --git a/packages/mcp/tests/test_bounded_catalog_pagination.py b/packages/mcp/tests/test_bounded_catalog_pagination.py new file mode 100644 index 00000000..405823e7 --- /dev/null +++ b/packages/mcp/tests/test_bounded_catalog_pagination.py @@ -0,0 +1,236 @@ +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from typing import Any + +import pytest + +from sagasmith_dnd_mcp.config import McpConfig +from sagasmith_dnd_mcp.server import _bounded_page, create_server + +COLLECTION_LIMIT_FIELDS = { + "actor_knowledge_query": "limit", + "branch_query": "limit", + "campaign_event": "limit", + "campaign_query": "limit", + "character_query": "limit", + "combat_query": "limit", + "content_pack": "limit", + "exposure": "limit", + "memory_query": "limit", + "module_draft": "limit", + "module_query": "limit", + "module_search": "top_k", + "npc_conversation": "limit", + "rule_search": "top_k", + "rulebook_draft": "limit", + "skill_query": "limit", + "snapshot_query": "limit", + "state_revision": "limit", +} + + +def _server(tmp_path: Path): + return create_server( + McpConfig( + home=tmp_path / "home", + database_url=None, + chroma_url=None, + chroma_path_override=None, + dnd_skills_dir=tmp_path / "dnd", + modulegen_skills_dir=tmp_path / "modulegen", + auto_seed_rules=False, + ) + ) + + +async def _raw(server, name: str, arguments: dict[str, Any]) -> tuple[Any, dict[str, Any]]: + content, structured = await server.call_tool(name, arguments) + assert isinstance(structured, dict) + return content, structured + + +def test_every_public_collection_facade_has_filter_limit_and_cursor_contract( + tmp_path: Path, +) -> None: + async def exercise() -> None: + server = _server(tmp_path) + tools = {tool.name: tool for tool in await server.list_tools()} + assert set(COLLECTION_LIMIT_FIELDS) <= set(tools) + + for tool_name, limit_field in COLLECTION_LIMIT_FIELDS.items(): + schema = tools[tool_name].input_schema + properties = schema["properties"] + assert "query" in properties, tool_name + assert "cursor" in properties, tool_name + assert properties["cursor"]["maxLength"] <= 1024, tool_name + assert limit_field in properties, tool_name + assert properties[limit_field]["maximum"] <= 100, tool_name + assert properties[limit_field]["minimum"] >= 1, tool_name + + # This is a fixed one-record capability catalog, not an unbounded + # collection, and therefore intentionally has no continuation cursor. + _, systems = await _raw(server, "system_list", {}) + assert len(systems) == 1 + + asyncio.run(exercise()) + + +def test_campaign_catalog_filter_and_cursor_are_bounded_stable_and_compatible( + tmp_path: Path, +) -> None: + async def exercise() -> None: + server = _server(tmp_path) + for name in ("Aster", "Birch", "Cedar", "Dogwood", "Elm"): + await _raw( + server, + "campaign_create", + {"name": name, "idempotency_key": f"pagination:{name}"}, + ) + + content, first = await _raw( + server, + "campaign_query", + {"view": "list", "limit": 2}, + ) + assert first["action"] == "list" + assert [item["name"] for item in first["result"]] == ["Aster", "Birch"] + assert first["page"] == { + "limit": 2, + "returned": 2, + "has_more": True, + "next_cursor": first["next_cursor"], + "total_count": 5, + } + assert isinstance(first["next_cursor"], str) + assert first["next_cursor"] not in {"2", "Aster", "Birch"} + assert content and json.loads(content[0].text)["result"] == first["result"] + + _, second = await _raw( + server, + "campaign_query", + {"view": "list", "limit": 2, "cursor": first["next_cursor"]}, + ) + assert [item["name"] for item in second["result"]] == ["Cedar", "Dogwood"] + first_ids = {item["id"] for item in first["result"]} + second_ids = {item["id"] for item in second["result"]} + assert not (first_ids & second_ids) + + # Opaque payload-based continuation remains compatible with the + # existing facade payload convention. + _, last = await _raw( + server, + "campaign_query", + { + "view": "list", + "limit": 2, + "payload": {"cursor": second["next_cursor"]}, + }, + ) + assert [item["name"] for item in last["result"]] == ["Elm"] + assert last["next_cursor"] is None + + _, filtered = await _raw( + server, + "campaign_query", + {"view": "list", "query": "cedar", "limit": 2}, + ) + assert [item["name"] for item in filtered["result"]] == ["Cedar"] + assert filtered["page"]["total_count"] == 1 + + asyncio.run(exercise()) + + +def test_cursor_is_bound_to_collection_and_filter_scope() -> None: + _, page = _bounded_page([{"id": 1}, {"id": 2}], scope="campaigns", limit=1) + cursor = page["next_cursor"] + assert cursor is not None + + with pytest.raises(ValueError, match="invalid for this query"): + _bounded_page([{"id": 1}, {"id": 2}], scope="characters", limit=1, cursor=cursor) + with pytest.raises(ValueError, match="invalid for this query"): + _bounded_page( + [{"id": 1}, {"id": 2}], + scope="campaigns", + query="changed", + limit=1, + cursor=cursor, + ) + with pytest.raises(ValueError, match="cursor and offset are mutually exclusive"): + _bounded_page( + [{"id": 1}, {"id": 2}], scope="campaigns", limit=1, cursor=cursor, offset=1 + ) + + +def test_event_and_revision_cursors_reach_beyond_first_hundred_records( + tmp_path: Path, +) -> None: + async def collect(server, name: str, arguments: dict[str, Any]) -> list[dict[str, Any]]: + values: list[dict[str, Any]] = [] + cursor: str | None = None + while True: + request = {**arguments, "limit": 100} + if cursor is not None: + request["cursor"] = cursor + _, structured = await _raw(server, name, request) + values.extend(structured["result"]) + cursor = structured["next_cursor"] + if cursor is None: + return values + + async def exercise() -> None: + server = _server(tmp_path) + _, created = await _raw( + server, + "campaign_create", + {"name": "Long history", "idempotency_key": "long-history"}, + ) + campaign_id = created["id"] + event_ids: set[str] = set() + for ordinal in range(125): + _, event = await _raw( + server, + "campaign_event", + { + "campaign_id": campaign_id, + "action": "add", + "payload": {"summary": f"Checkpoint {ordinal:03d}"}, + "idempotency_key": f"long-history:{ordinal}", + }, + ) + event_ids.add(event["result"]["id"]) + + events = await collect( + server, + "campaign_event", + {"campaign_id": campaign_id, "action": "list"}, + ) + assert len(events) == len({item["id"] for item in events}) == 125 + assert {item["id"] for item in events} == event_ids + + current = created + for ordinal in range(105): + _, changed = await _raw( + server, + "campaign_change", + { + "campaign_id": campaign_id, + "action": "update", + "payload": {"description": f"Revision checkpoint {ordinal:03d}"}, + "expected_revision": current["revision"], + "idempotency_key": f"revision-history:{ordinal}", + }, + ) + current = changed["result"] + + revisions = await collect( + server, + "state_revision", + {"campaign_id": campaign_id, "action": "history"}, + ) + assert len(revisions) > 100 + assert len(revisions) == len({item["sequence"] for item in revisions}) + + asyncio.run(exercise()) diff --git a/packages/mcp/tests/test_read_only_evaluations.py b/packages/mcp/tests/test_read_only_evaluations.py index 20ad50e3..cceb41ca 100644 --- a/packages/mcp/tests/test_read_only_evaluations.py +++ b/packages/mcp/tests/test_read_only_evaluations.py @@ -1,42 +1,108 @@ from __future__ import annotations import asyncio +from collections import Counter from pathlib import Path +from typing import Any, Callable from xml.etree import ElementTree from sagasmith_dnd_mcp.config import McpConfig from sagasmith_dnd_mcp.server import create_server +CAMPAIGNS = ( + ( + "Amber Observatory", + "Wind-scoured observatory above salt cliffs, where a sealed lens tracks old stars.", + ( + ("Aria", "pc", "A patient cartographer."), + ("Borin", "npc", "A veteran guide."), + ("Lumen", "npc", "A lantern keeper."), + ("Nyx", "npc", "Keeper of the sealed lens."), + ("Glass Drake", "monster", "A crystal-scaled sentinel."), + ), + ), + ( + "Bronze Citadel", + "Bronze walls guard a river archive and its forgotten gatehouse.", + ( + ("Zephyr", "pc", "A storm-touched envoy."), + ("Mira", "npc", "The citadel archivist."), + ("Ash Wyrm", "monster", "A creature beneath the gate."), + ), + ), + ( + "Cobalt Labyrinth", + "Flooded passages connect a moonlit market to the lower vaults.", + ( + ("Cato", "pc", "A cautious delver."), + ("Dena", "pc", "A mapmaker who reads currents."), + ("Edda", "npc", "A market guide."), + ("Fenn", "npc", "A keeper of flood charts."), + ("Gale", "npc", "A broker for the moonlit market."), + ("Gloom Ooze", "monster", "A thing in the cistern."), + ("Harrow Beast", "monster", "A hunter in the lower vault."), + ), + ), + ( + "Ivory Archive", + "Silent stacks preserve crown treaties and testimony from vanished courts.", + ( + ("Iona", "pc", "A treaty scholar."), + ("Jori", "npc", "A meticulous indexer."), + ("Kestrel", "npc", "A courier of sealed records."), + ("Morrow", "npc", "A retired court witness."), + ("Nacre", "npc", "The archive conservator."), + ), + ), + ( + "Jade Harbor", + "Tidal bells guide ships through green fog toward a sheltered quay.", + ( + ("Orin", "pc", "A patient navigator."), + ("Petra", "pc", "A harbor scout."), + ("Quill", "npc", "The keeper of tidal bells."), + ("Rhea", "npc", "A quay registrar."), + ), + ), + ( + "Violet March", + "A caravan road crosses fields of violet ash beneath an empty watchtower.", + ( + ("Sable", "pc", "A caravan outrider."), + ("Taro", "npc", "The last watchtower keeper."), + ("Umbra", "monster", "An ash-shadow predator."), + ("Ulan", "npc", "A caravan quartermaster."), + ("Vale", "monster", "A burrowing road hunter."), + ("Wren", "monster", "A winged scavenger."), + ), + ), +) + -async def _call(server, name: str, arguments: dict | None = None): +async def _raw(server, name: str, arguments: dict[str, Any] | None = None) -> dict[str, Any]: _, structured = await server.call_tool(name, arguments or {}) - if isinstance(structured, dict): - return structured.get("result", structured) + assert isinstance(structured, dict) return structured +async def _call(server, name: str, arguments: dict[str, Any] | None = None) -> Any: + structured = await _raw(server, name, arguments) + return structured.get("result", structured) + + async def _fixture(server) -> None: - campaigns: dict[str, dict] = {} - for name in ("Alpha Expedition", "Beta Citadel"): + campaigns: dict[str, dict[str, Any]] = {} + for name, description, _ in CAMPAIGNS: campaigns[name] = await _call( server, "campaign_create", - {"name": name, "idempotency_key": f"evaluation:{name}"}, + { + "name": name, + "description": description, + "idempotency_key": f"evaluation:{name}", + }, ) - actors = { - "Alpha Expedition": [ - ("Aria", "pc", "A patient cartographer."), - ("Borin", "npc", "A veteran guide."), - ("Lumen", "npc", "A lantern keeper."), - ("Nyx", "npc", "Custodian of the sealed observatory."), - ], - "Beta Citadel": [ - ("Zephyr", "pc", "A storm-touched envoy."), - ("Mira", "npc", "The citadel archivist."), - ("Ash Wyrm", "monster", "A creature beneath the gate."), - ], - } - for campaign_name, roster in actors.items(): + for campaign_name, _, roster in CAMPAIGNS: for name, character_type, summary in roster: await _call( server, @@ -54,16 +120,49 @@ async def _fixture(server) -> None: ) -async def _solve(server) -> list[str]: - campaigns = await _call(server, "campaign_query", {"view": "list"}) - campaigns = sorted(campaigns, key=lambda item: item["name"]) - rosters: dict[str, list[dict]] = {} - details: list[dict] = [] +async def _paged( + server, + name: str, + arguments: dict[str, Any], + stats: Counter[str], +) -> list[dict[str, Any]]: + values: list[dict[str, Any]] = [] + cursor: str | None = None + while True: + request = {**arguments, "limit": 2} + if cursor is not None: + request["cursor"] = cursor + stats["continuations"] += 1 + structured = await _raw(server, name, request) + stats["calls"] += 1 + page_values = structured.get("result") + page = structured.get("page") + assert isinstance(page_values, list) + assert isinstance(page, dict) + assert page["returned"] == len(page_values) <= 2 + assert structured.get("next_cursor") == page.get("next_cursor") + values.extend(page_values) + cursor = page.get("next_cursor") + if cursor is None: + assert page["has_more"] is False + assert page["total_count"] == len(values) + return values + assert page["has_more"] is True + + +async def _explore_fixture(server) -> tuple[dict[str, Any], Counter[str]]: + """Perform a fresh, paginated, multi-hop exploration for one QA pair.""" + + stats: Counter[str] = Counter() + campaigns = await _paged(server, "campaign_query", {"view": "list"}, stats) + rosters: dict[str, list[dict[str, Any]]] = {} + details: list[dict[str, Any]] = [] for campaign in campaigns: - roster = await _call( + roster = await _paged( server, "character_query", {"view": "list", "payload": {"campaign_id": campaign["id"]}}, + stats, ) rosters[campaign["id"]] = roster for actor in roster: @@ -74,36 +173,125 @@ async def _solve(server) -> list[str]: {"view": "get", "payload": {"character_id": actor["id"]}}, ) ) - campaign_by_id = {item["id"]: item for item in campaigns} - all_actors = [actor for roster in rosters.values() for actor in roster] - largest = max(campaigns, key=lambda item: len(rosters[item["id"]])) - last_pc = max(actor["name"] for actor in all_actors if actor["character_type"] == "pc") - monster = next(actor for actor in all_actors if actor["character_type"] == "monster") - npc_count = sum(actor["character_type"] == "npc" for actor in all_actors) - balanced = next( + stats["calls"] += 1 + systems = await _call(server, "system_list") + stats["calls"] += 1 + return { + "campaigns": campaigns, + "campaign_by_id": {item["id"]: item for item in campaigns}, + "rosters": rosters, + "details": details, + "systems": systems, + }, stats + + +def _role_counts(data: dict[str, Any], campaign: dict[str, Any]) -> Counter[str]: + return Counter( + actor["character_type"] for actor in data["rosters"][campaign["id"]] + ) + + +def _answer_largest_npc_roster(data: dict[str, Any]) -> str: + return max( + data["campaigns"], + key=lambda campaign: (_role_counts(data, campaign)["npc"], campaign["name"]), + )["name"] + + +def _answer_largest_monster_roster(data: dict[str, Any]) -> str: + return max( + data["campaigns"], + key=lambda campaign: (_role_counts(data, campaign)["monster"], campaign["name"]), + )["name"] + + +def _answer_two_pc_largest_roster(data: dict[str, Any]) -> str: + candidates = [ campaign - for campaign in campaigns - if sum(actor["character_type"] == "pc" for actor in rosters[campaign["id"]]) - == sum(actor["character_type"] == "npc" for actor in rosters[campaign["id"]]) + for campaign in data["campaigns"] + if _role_counts(data, campaign)["pc"] == 2 + ] + return max(candidates, key=lambda item: len(data["rosters"][item["id"]]))["name"] + + +def _answer_last_pc(data: dict[str, Any]) -> str: + return max( + actor["name"] + for roster in data["rosters"].values() + for actor in roster + if actor["character_type"] == "pc" ) - observatory = next( - actor for actor in details if "sealed observatory" in actor["summary"].casefold() + + +def _answer_lens_campaign_slug(data: dict[str, Any]) -> str: + keeper = next( + actor for actor in data["details"] if "sealed lens" in actor["summary"].casefold() ) - beta = next(item for item in campaigns if item["name"] == "Beta Citadel") - systems = await _call(server, "system_list") - system_names = {item["id"]: item["display_name"] for item in systems} - return [ - largest["name"], - last_pc, - campaign_by_id[monster["campaign_id"]]["name"], - str(npc_count), - str(len(all_actors)), - balanced["name"], - observatory["name"], - str(len({actor["character_type"] for actor in rosters[beta["id"]]})), - system_names[campaigns[0]["system_id"]], - campaigns[0]["slug"], + return data["campaign_by_id"][keeper["campaign_id"]]["slug"] + + +def _answer_tidal_first_npc(data: dict[str, Any]) -> str: + campaign = next( + item for item in data["campaigns"] if "tidal bells" in item["description"].casefold() + ) + return min( + actor["name"] + for actor in data["rosters"][campaign["id"]] + if actor["character_type"] == "npc" + ) + + +def _answer_no_monster_ratio(data: dict[str, Any]) -> str: + candidates = [ + campaign + for campaign in data["campaigns"] + if _role_counts(data, campaign)["monster"] == 0 ] + return max( + candidates, + key=lambda campaign: ( + _role_counts(data, campaign)["npc"] / _role_counts(data, campaign)["pc"], + campaign["name"], + ), + )["name"] + + +def _answer_largest_roster_classifications(data: dict[str, Any]) -> str: + campaign = max( + data["campaigns"], key=lambda item: len(data["rosters"][item["id"]]) + ) + return str(len(_role_counts(data, campaign))) + + +def _answer_outer_campaign_system(data: dict[str, Any]) -> str: + campaigns = sorted(data["campaigns"], key=lambda item: item["name"]) + assert campaigns[0]["system_id"] == campaigns[-1]["system_id"] + displays = {item["id"]: item["display_name"] for item in data["systems"]} + return displays[campaigns[0]["system_id"]] + + +def _answer_monsters_equal_others(data: dict[str, Any]) -> str: + campaign = next( + item + for item in data["campaigns"] + if _role_counts(data, item)["monster"] + == _role_counts(data, item)["pc"] + _role_counts(data, item)["npc"] + ) + return campaign["slug"] + + +SOLVERS: tuple[Callable[[dict[str, Any]], str], ...] = ( + _answer_largest_npc_roster, + _answer_largest_monster_roster, + _answer_two_pc_largest_roster, + _answer_last_pc, + _answer_lens_campaign_slug, + _answer_tidal_first_npc, + _answer_no_monster_ratio, + _answer_largest_roster_classifications, + _answer_outer_campaign_system, + _answer_monsters_equal_others, +) def test_builder_evaluations_are_independent_read_only_and_actually_solved( @@ -112,7 +300,7 @@ def test_builder_evaluations_are_independent_read_only_and_actually_solved( evaluation_path = Path(__file__).parents[1] / "evaluations" / "read_only.xml" root = ElementTree.parse(evaluation_path).getroot() pairs = root.findall("qa_pair") - assert len(pairs) >= 10 + assert len(pairs) == len(SOLVERS) == 10 questions = [str(pair.findtext("question") or "").strip() for pair in pairs] answers = [str(pair.findtext("answer") or "").strip() for pair in pairs] assert len(questions) == len(set(questions)) @@ -137,6 +325,15 @@ async def exercise() -> None: assert annotations is not None assert annotations.read_only_hint is True assert annotations.idempotent_hint is True - assert await _solve(server) == answers + + observed: list[str] = [] + for solve in SOLVERS: + # Every QA pair starts from a fresh catalog traversal. No answer or + # intermediate result from another pair is reused. + data, stats = await _explore_fixture(server) + observed.append(solve(data)) + assert stats["calls"] >= 35 + assert stats["continuations"] >= 7 + assert observed == answers asyncio.run(exercise()) diff --git a/packages/mcp/tests/test_statblock_import_mcp.py b/packages/mcp/tests/test_statblock_import_mcp.py index c3314391..8a1c86c8 100644 --- a/packages/mcp/tests/test_statblock_import_mcp.py +++ b/packages/mcp/tests/test_statblock_import_mcp.py @@ -683,7 +683,7 @@ async def exercise() -> None: "payload": { "job_id": job_id, "kind": "chunks", - "limit": 200, + "limit": 100, }, }, )