diff --git a/.consiliency/evidence/mutation-175.md b/.consiliency/evidence/mutation-175.md new file mode 100644 index 00000000..de5aac8d --- /dev/null +++ b/.consiliency/evidence/mutation-175.md @@ -0,0 +1,127 @@ +# Mutation evidence — Consiliency/pmcp#175 + +Protocol: `__pycache__` purged and `PYTHONDONTWRITEBYTECODE=1` on every +apply/restore, per the tooling note in `mutation-180.md`. Stale bytecode +fabricated a false RED during #184 and can equally fabricate a false GREEN. + +Every exit code below was **captured from the shell** (`echo $?` immediately +after the run), not read off a summary line. The numbers here were produced by +re-running both mutations after the board round, on the branch as it stands — +not transcribed from an earlier session. + +## 1. The truncation boundary — item 1, the mutant that survived nine others + +The reason #175 item 1 exists. Mutating the tool-limit guard in +`_parse_tool_entries` (`src/pmcp/client/manager.py`) from `>=` to `>` **survived +the entire existing `tests/test_client_manager.py`** and was the sole survivor +of a nine-mutant battery. The off-by-one lets a downstream put `limit + 1` tools +in the catalog — one more than the bound, every time, silently. + + - if len(entries) >= limit: + + if len(entries) > limit: + +Command, run at each of the three tree states: + + find . -name __pycache__ -type d -not -path "./.venv/*" -exec rm -rf {} + + PYTHONDONTWRITEBYTECODE=1 uv run --no-sync pytest -q \ + tests/test_client_manager.py -k "TestToolLimitIsEnforced" + +| tree state | result | exit | +|---|---|---| +| unchanged (`>=`) | `4 passed, 225 deselected in 1.40s` | **0** | +| mutant applied (`>`) | `2 failed, 2 passed, 225 deselected in 1.45s` | **1** | +| restored (`>=`), `git diff` empty | `4 passed, 225 deselected in 1.25s` | **0** | + +The mutant's failure, which is the whole point — the catalog took six tools +under a limit of five: + + FAILED ...::TestToolLimitIsEnforced::test_no_more_than_the_limit_is_ever_indexed[6] + FAILED ...::TestToolLimitIsEnforced::test_truncation_keeps_the_first_entries_and_says_so + E assert 6 == 5 + E + where 5 = <...TestToolLimitIsEnforced object ...>.LIMIT + +**Ordering matters and was honoured.** The test was written and committed +(`fde84cc`) **against unchanged code, before the mutation was applied** — a test +written after the rest of the diff cannot demonstrate it would have caught the +bug. Only the `limit + 1` case distinguishes `>=` from `>`, which is why the +test asserts *exact* counts at `limit - 1`, `limit` and `limit + 1` rather than +the weaker "fewer than offered", which the mutant passes. + +## 2. `adopt_process`'s removal deleted — item 2 + +Item 2 added one line to `adopt_process`. A test that merely asserted the +removal *method was called* would pass just as happily with the call in the +wrong place, so the tests assert on **catalog contents** — and this mutation is +what demonstrates they do. + + - self._remove_server_indexes(name) + +Command, at each of the three tree states: + + find . -name __pycache__ -type d -not -path "./.venv/*" -exec rm -rf {} + + PYTHONDONTWRITEBYTECODE=1 uv run --no-sync pytest -q \ + tests/test_client_manager.py -k "TestAdoptProcessRemovesStaleIndexesFirst" + +| tree state | result | exit | +|---|---|---| +| unchanged (line present) | `4 passed, 225 deselected in 1.24s` | **0** | +| mutant applied (line deleted) | `2 failed, 2 passed, 225 deselected in 1.52s` | **1** | +| restored, `git diff` empty | `4 passed, 225 deselected in 1.32s` | **0** | + +The mutant reproduces the defect exactly — a stale tool from a previous listing +surviving the adopt, still routable: + + FAILED ...::TestAdoptProcessRemovesStaleIndexesFirst::test_a_prior_listings_tools_do_not_survive_the_adopt + FAILED ...::TestAdoptProcessRemovesStaleIndexesFirst::test_resources_and_prompts_are_cleared_too + E AssertionError: adopt_process indexed on top of the previous catalog + E assert {'srv::fresh', 'srv::stale'} == {'srv::fresh'} + E Extra items in the left set: 'srv::stale' + +The two tests that stay green under the mutant are the ones that *should*: +`test_another_servers_entries_are_untouched` and +`test_adopting_a_name_with_no_prior_index_is_unchanged` pin that the removal is +per-server and harmless when there is nothing to remove — neither is a +discriminating test for this line, and neither pretends to be. + +## A regression the suite caught that no mutation would have + +Recorded because it is the more instructive failure of the two, and because it +argues for keeping guard tests that look redundant. + +Item 5's first implementation moved the catalog **write** and the count together +into a module-level helper, `_apply_entries(catalog, name, kind, entries)`. +`tests/runtime/test_publisher_coverage.py` is an AST honesty guard: it parses +`client/manager.py`, attributes every `self._tools` / `self._resources` / +`self._prompts` write to its enclosing method, and fails if one appears outside +the five publishing mutators. A helper that receives the dict as a *parameter* +is invisible to that walk — so the refactor did not trip the guard, it +**silently emptied** it: + + E AssertionError: expected writes from {'_index_tools', '_index_resources', + '_index_prompts', '_remove_server_indexes', '_disconnect_all_unlocked'}, + saw {'_remove_server_indexes', '_disconnect_all_unlocked', '__init__'} + +The three `_index_*` methods had stopped appearing as catalog writers at all. +Nothing about the observable behaviour changed — counts and logs were identical +— so no mutation of the *product* code would have surfaced this. Only a guard +asserting on the shape of the source did. + +Fixed in `b5d0609`: the write goes back inside each `_index_*`, where the guard +can attribute it, and the helper — now `_distinct_indexed(name, kind, entries)` +— only counts and logs. Its docstring records why it must not take the catalog +dict, so the next reader does not re-make the same simplification. + +The lesson generalises: a guard that asserts *"these five functions must still +do X"* protects against refactors that a behavioural test cannot see, and its +apparent redundancy is exactly what makes it load-bearing. + +## What is deliberately not mutation-tested + +Item 3 changes **log wording only** and adds no schema bound. There is no +behavioural boundary to mutate: `LimitsPolicy(max_tools_per_server=0)` must +continue to **validate**, because `PolicyManager._load_policy(..., fatal=False)` +swallows validation errors and falls back to the allow-all default — so +rejecting the value would silently discard an operator's entire policy file. +That fail-open is #202. The tests assert on the emitted message (the zero limit +is named, "unparseable" is absent) and pin that `0` still validates, which is +the strongest available statement about a change that alters no behaviour. diff --git a/.consiliency/plans/detailed-catalog-indexing-hardening-20260827-1745.md b/.consiliency/plans/detailed-catalog-indexing-hardening-20260827-1745.md new file mode 100644 index 00000000..4696293b --- /dev/null +++ b/.consiliency/plans/detailed-catalog-indexing-hardening-20260827-1745.md @@ -0,0 +1,190 @@ +# Detailed plan: close the five catalog-indexing findings + +> **Revision 2 (2026-08-27).** Boarded 2 AGREE / 1 DISAGREE. Rev 1's item 3 would +> have introduced a **fail-open security regression**, and its item 4 named a +> helper that rejects every valid tool. Both corrected below. + +## Task + +Close Consiliency/pmcp#175 — five findings split out of #173 so that issue could +close on its own subject. None is blocking; all are recorded so they stay visible. + +**Scope note.** Five distinct changes is at the upper end of what one bounded +plan should carry. They are kept together because each is a few lines in two +files (`manager.py`, `types.py`), they share one reviewer context (catalog +indexing), and splitting would cost five plan/board/PR cycles for changes smaller +than the ceremony. If the board disagrees, item 1 (the mutation-surviving test) +is the one worth its own change — it is the only item that is a *missing test* +rather than a code fix. + +## Research summary + +Verified in this worktree: + +- `_index_tools` (`manager.py:~1611-1621`): `entries` is a **list** of + `(tool_id, tool_info)` tuples. The loop does `self._tools[tool_id] = tool_info`, + so two entries sharing an identity overwrite — but `indexed = len(entries)` + counts the **list**, so both are counted while one lands. The docstring on the + sibling `_index_resources` (`:1633-1635`) states the opposite in as many words: + *"The count returned is what was actually indexed, not what was offered, so a + caller reporting it is not overstating the catalog."* That claim is false + exactly when identities collide. A documented guarantee contradicted by the + code is worse than an undocumented gap — a reader trusts it. +- `LimitsPolicy.max_tools_per_server` (`types.py:960`) is a bare `int = 100` with + no bound, so `0` validates. +- `adopt_process` (`manager.py:3134`) indexes with no preceding + `_remove_server_indexes`; the connect and reconcile paths at `:1238`, `:2094` + and `:2126` all remove first. +- `input_schema = tool.get("inputSchema", {})` (`manager.py:616`) sits directly + beside `_required_identity(tool, "name")` — the same function that #172 added + precisely to stop manufacturing missing values. + +## Changes + +### `src/pmcp/client/manager.py` (modify) + +- `_index_tools` / `_index_resources` / the third `_index_*` — modify — count + what actually landed, not what was offered. Simplest correct form: return + `len({tool_id for tool_id, _ in entries})`, or count insertions. **Item 5.** +- `_index_resources` docstring (`:1633-1635`) — modify — the claim becomes true + once the count is fixed; keep the sentence and let the code earn it rather than + deleting the promise. +- A duplicate identity within one listing — modify — log once at DEBUG naming the + colliding id. Silent last-write-wins is the failure the count fix makes + *visible*; a log makes it *diagnosable*. **Item 5.** +- `adopt_process` (`:3134`) — modify — call `_remove_server_indexes(name)` before + indexing, matching every other indexing path. **Item 2.** Chosen over amending + the docstrings because three call sites already do it and the odd one out is + the surprise; making the code uniform is cheaper to hold in the head than an + exception written down in two places. +- `input_schema = tool.get("inputSchema", {})` (`:616`) — modify — treat a + missing `inputSchema` as **unparseable**, so the entry is skipped and logged + like any other unparseable entry (see the helper note below). + **Item 4.** This is the deliberate decision #175 asks for: MCP requires + `inputSchema` on a tool, and manufacturing an accept-anything schema + misrepresents the tool to every caller. Consistent with #172's ruling on + manufactured identities. The board agreed it should ship as skip-and-log rather + than as a documented default. + + **Do NOT call `_required_identity(tool, "inputSchema")`.** Verified: that helper + requires `isinstance(value, str) and value` (`manager.py:530-534`), and an + `inputSchema` is an **object** — a literal call would reject every valid tool + in the catalog. Add a sibling required-**object** check that raises inside the + same per-entry `try`, so the existing skip-and-log path handles it unchanged. + + **Fixture fallout, on the change list rather than discovered at runtime:** + `tests/mcp2x/test_catalog_publishers.py` indexes `{"name": "t1"}` with no + `inputSchema` at lines **130, 140, 193, 218, 278, 310** and asserts `== 1`. + Those fixtures need a valid `inputSchema`, as do the new item 1 and item 5 + fixtures — otherwise item 4 silently zeroes their counts and the failure looks + like a bug in items 1 and 5. + +### `src/pmcp/types.py` (modify) + +- **Item 3 — do NOT add `Field(ge=1)`.** Fix the *log*, not the schema: when the + limit truncates a listing to zero entries, say that the limit is zero rather + than "all entries unparseable". + + **WAS WRONG (rev 1).** Verified: `PolicyManager._load_policy(..., fatal=False)` + — the auto-discovery path — swallows **any** validation exception and leaves + `self._policy` as the default `GatewayPolicy()`, which is **allow-all** + (`policy/policy.py:59-77`). So adding `ge=1` would make every policy file + containing `max_tools_per_server: 0` schema-invalid, silently discarding that + file **in its entirety** — allow/deny lists, limits and redaction all reverting + to permissive defaults. That converts a cosmetic log fix into a security + regression, which is a spectacularly bad trade. + + The underlying fail-open is filed separately as **#202**; it is not this + change's to fix, and folding it in would hide it. Any future tightening of the + policy schema carries the same risk until #202 is resolved. + +### `tests/test_client_manager.py` (modify) + +- `TestToolLimitIsEnforced` — add — index **more** entries than + `max_tools_per_server` and assert exactly `limit` land in the catalog. + **Item 1.** This is the item with teeth: a mutation of the boundary + (`len(entries) >= limit` → `>`) **survived the entire existing file** and was + the sole survivor of nine. The test must therefore be written so that mutation + fails — i.e. it must pin the boundary, not merely that truncation happens. + Assert at `limit`, `limit + 1`, and `limit - 1`. +- `TestDuplicateIdentitiesAreNotDoubleCounted` — add — two entries sharing an id; + assert the returned count equals the number in the catalog. **Item 5.** +- `TestAdoptProcessRemovesFirst` — add — index, adopt, assert no stale entry + survives. **Item 2.** +- `TestMissingInputSchemaIsUnparseable` — add — **Item 4.** +- `TestZeroLimitLogsAccurately` — add — with `max_tools_per_server=0`, the + emitted message names the zero limit and does **not** say the entries were + unparseable; `LimitsPolicy(max_tools_per_server=0)` still validates. **Item 3.** + +## Documentation impact + +- `CHANGELOG.md` — add — one `### Fixed` entry naming all five; item 4 is a + behaviour change (a tool without `inputSchema` is now skipped rather than + accepted with a permissive schema) and must be called out as such, not buried. +- No README change: none of this alters user-facing configuration. The policy + schema is deliberately left untouched (see item 3 and #202). + +## Dependencies & order + +1. Item 1's test **first, against unchanged code** — confirm it passes, then + apply the boundary mutation and confirm it fails. A test written after the + fix cannot prove it would have caught the bug. +2. Items 3 (log wording only) and 5 (independent, mechanical). +3. Item 2 (`adopt_process`), then item 4 (`inputSchema`) — item 4 is the only + behaviour change and belongs last so a bisect lands on it cleanly. + +## Verification + +```bash +uv run pytest -q tests/test_client_manager.py +# Item 1 must be mutation-proven, not assumed: +# flip `>= limit` to `> limit` in the truncation, run the new test, expect RED, +# restore, expect GREEN. Record both exit codes. +uv run pytest -q +uv run ruff check src/ tests/ && uv run ruff format --check src/ tests/ && uv run mypy src/ +# Scoped to `src/ tests/`, matching CI's `lint` job. The repo-wide form was +# over-broad: `ruff format --check .` is red on +# `diagnostics/issue-79-1b/repro_client.py` and `slow_server.py`, which are +# known pre-existing debt untouched by this change. Reformatting them from a +# catalog-indexing PR would be scope creep and would muddy the diff, so the +# check is scoped rather than the files rewritten. +# (Rev 1 listed a 98-server manifest check here. That is npm identity +# resolution, unrelated to catalog indexing, and is not coverage for dropped +# tools — removed rather than left as false reassurance.) +``` + +Edge cases: a listing that is entirely duplicates; `adopt_process` on a name with +no prior index; a tool with `inputSchema: null` (distinct from absent); a +listing exactly at the limit. + +## Acceptance criteria + +- [ ] The boundary mutation `>= limit` → `> limit` makes the new test **fail** — + recorded before/after exit codes. Item 1 exists because that mutation + survived nine others; a test that does not kill it has not closed the + finding. +- [ ] For a listing with duplicate identities, each `_index_*` returns a count + equal to the number of that server's entries in **its own** catalog — + `_index_tools` against `_tools`, `_index_resources` against `_resources`, + `_index_prompts` against `_prompts`. **WAS WRONG (rev 1):** the criterion + compared all three against `self._tools`, so it could not prove the guarantee + for resources or prompts at all. +- [ ] With `max_tools_per_server=0`, the log says the limit is zero and does + **not** claim the entries were unparseable — asserted on the emitted + message. `LimitsPolicy(max_tools_per_server=0)` must still **validate**, + since rejecting it is what triggers #202's fail-open. +- [ ] `adopt_process` leaves no entry that a prior index put there — asserted on + catalog contents, not by checking that the method was called. +- [ ] A tool dict without `inputSchema` is skipped and logged as unparseable, + and one with `inputSchema` present is indexed unchanged. +- [ ] Full suite green; no change to the 98-server manifest resolution. + +## Non-goals + +- Reworking `_parse_tool_entries`' per-entry error handling, which #172 settled. +- Any change to catalog *pagination*, which #173/#174 closed. + +## Execution Policy + +- execute: effort=medium, reason=five small changes, one of which (item 4) + changes what is accepted from a server and needs the CHANGELOG to say so diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5178ec..bf5bbdc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- The truncation boundary at `max_tools_per_server` is now pinned by tests at + `limit - 1`, `limit` and `limit + 1`. Mutating the guard from `>=` to `>` — + which lets a server put one more tool in the catalog than the bound allows — + previously survived the whole of `tests/test_client_manager.py`. (#175) + +### Changed +- **A downstream tool that declares no `inputSchema` is now skipped instead of + indexed.** This is a behaviour change, and the only one in this set. The + indexer used to substitute `{}` for a missing `inputSchema` — and `{}` is not + "we do not know", it is "any arguments at all are valid", published under the + server's name to every caller and every model reading the catalog. MCP + requires `inputSchema` on a tool, so a tool without one is a tool we could not + read, and such an entry now takes the same route as any other unparseable + entry: skipped, logged, costing only itself. A listing in which *no* tool + parses is treated as a failed listing, so the server's previous tools are kept + rather than reported removed. An explicitly empty `inputSchema: {}` is still + accepted — the server said "any arguments", and that is an answer; only the + absence, and any non-object value such as `null`, is unreadable. A server that + omits `inputSchema` therefore loses that tool from the catalog where it + previously appeared with a permissive schema. (#175) + ### Fixed - **`derive_npm_flags.py --verify` no longer reports host-enumerated npm config types as table drift.** The npm flag tables are the node-less fallback for @@ -41,6 +63,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 test now also holds the recorded schema fixture and the committed tables to each other, so regenerating one without the other cannot pass silently. (#193) +- **`_index_tools`/`_index_resources`/`_index_prompts` no longer overstate the + catalog.** `_index_resources` documented that "the count returned is what was + actually indexed, not what was offered", while all three returned the length + of the parsed list. Two entries sharing an identity are two list items and one + catalog key, so the count was wrong by exactly the number of collisions. The + count is now of entries that actually landed, and each collision is logged at + DEBUG naming the id. (#175) +- **`adopt_process` now clears the server's catalog entries before indexing**, + like every other path into the indexers. Adopting a server previously indexed + under the same name left the earlier listing's tools in the catalog beside the + new ones — entries the adopted process does not serve, still routable. (#175) +- **A `max_tools_per_server` of `0` is no longer reported as a malformed + listing.** A zero limit empties the parse result before any entry is examined, + so reconciliation announced "Every tools entry in the listing was + unparseable" — blaming the downstream for a decision the gateway's own policy + file made. Both that message and the parser's truncation warning now name the + limit. Deliberately *not* fixed by adding a schema bound: `LimitsPolicy` still + accepts `0`, because policy auto-discovery swallows validation errors and + falls back to an allow-all default, so rejecting the value would silently + discard the operator's entire policy file (#202). (#175) ## [2.6.0] - 2026-08-27 diff --git a/src/pmcp/client/manager.py b/src/pmcp/client/manager.py index 4ca97e6e..79c65b49 100644 --- a/src/pmcp/client/manager.py +++ b/src/pmcp/client/manager.py @@ -534,6 +534,40 @@ def _required_identity(entry: Any, key: str) -> str: return value +def _required_object(entry: Any, key: str) -> dict[str, Any]: + """The object-valued field a catalog entry cannot be indexed without. + + `_required_identity`'s rule, for a field whose value is an object rather + than a string. It exists as a sibling and not as a flag on that helper + because that one requires `isinstance(value, str) and value`: called for + `inputSchema` it would reject every valid tool in the catalog. + + `tool.get("inputSchema", {})` manufactured an accept-anything schema for a + tool that declared none -- and `{}` is not "we do not know", it is "any + arguments at all are valid", which is published to every caller and to + every model reading the catalog. MCP requires `inputSchema` on a tool, so a + tool without one is a tool we could not read, and #172 already settled what + to do with those: skip it and say so, rather than invent the missing value. + + Raised inside the parser's per-entry `try`, so the entry lands in the + existing skip-and-log path and a listing of nothing but such entries lands + in `_reconcile_once`'s offered-but-none-parseable rule -- prior entries + kept, nothing published. + + An explicitly empty `{}` is accepted: the server said "any arguments", and + that is an answer. `null`, a string, a list and an absent field are not. + """ + if not isinstance(entry, dict): + raise TypeError(f"catalog entry is {type(entry).__name__}, not an object") + value = entry.get(key) + if not isinstance(value, dict): + raise ValueError( + f"catalog entry has no usable `{key}`: expected an object, got " + f"{type(value).__name__}" + ) + return value + + def _listing_entries(name: str, kind: str, result: Any) -> list[dict[str, Any]] | None: """The entries a `*/list` reply offered, or `None` if it offered none readably. @@ -582,6 +616,45 @@ def _schema_dialect(*schemas: dict[str, Any] | None) -> str: return DEFAULT_SCHEMA_DIALECT +def _distinct_indexed(name: str, kind: str, entries: list[tuple[str, Any]]) -> int: + """How many catalog keys `entries` actually occupies, logging collisions. + + `len(entries)` is the wrong number and `_index_resources` documents the + right one: "the count returned is what was actually indexed, not what was + offered, so a caller reporting it is not overstating the catalog." Two + entries that share an identity are two list items but one dict key -- the + second overwrites the first -- so the list length overstates the catalog by + exactly the collisions, and a promise contradicted by the code is worse + than no promise, because a reader trusts it. Counting distinct identities + makes the sentence true. (#175 item 5.) + + The collision is logged rather than silently absorbed. Last-write-wins is + the downstream's bug, not ours, and the honest count makes it *visible* -- + the log is what makes it *diagnosable*. DEBUG because a server can only + reach here by offering a duplicate, and the operator who cares is already + looking. + + This counts and logs; it deliberately does **not** write. An earlier draft + took the catalog dict as a parameter and did both, which moved every write + out of the three `_index_*` methods and straight past + `tests/runtime/test_publisher_coverage.py` -- an AST guard that attributes + each `self._tools`/`self._resources`/`self._prompts` write to its enclosing + method and fails if one appears outside the five publishing mutators. A + helper taking the dict as an argument is invisible to it, so the refactor + would have silently disarmed the guard rather than tripped it. The write + stays where the guard can see it. + """ + seen: set[str] = set() + for entry_id, _entry in entries: + if entry_id in seen: + logger.debug( + f"[{name}] Duplicate {kind} id {entry_id!r} in one listing; " + "the later entry wins and the count reflects one entry, not two" + ) + seen.add(entry_id) + return len(seen) + + def _parse_tool_entries( name: str, tools: list[dict[str, Any]], limit: int ) -> list[tuple[str, ToolInfo]]: @@ -606,14 +679,25 @@ def _parse_tool_entries( } for tool in tools: if len(entries) >= limit: - logger.warning(f"Server {name} has more than {limit} tools, truncating") + if limit < 1: + # "has more than 0 tools, truncating" is true and useless: it + # reads as a server that overran a bound, when what happened is + # that the bound admits nothing. Name the limit instead -- the + # operator's next move is to fix their policy file, and the log + # should point at it. (#175 item 3.) + logger.warning( + f"Server {name} offered {len(tools)} tools but " + f"max_tools_per_server is {limit}, so none were indexed" + ) + else: + logger.warning(f"Server {name} has more than {limit} tools, truncating") break try: tool_name = _required_identity(tool, "name") tool_id = make_tool_id(name, tool_name) description = tool.get("description", "") - input_schema = tool.get("inputSchema", {}) + input_schema = _required_object(tool, "inputSchema") output_schema = tool.get("outputSchema") tool_info = ToolInfo( @@ -1607,6 +1691,12 @@ def _index_tools( anything -- hand the result straight in, so each listing is parsed once and each unparseable entry is logged once. Callers with a raw listing omit it and this method parses for them. + + The write stays here, in the method the publisher-coverage AST guard + attributes it to. The *count* comes from `_distinct_indexed`, and is a + count of entries that actually landed, so duplicate identities in one + listing -- one catalog key, whatever the list length -- are counted + once. """ entries = ( _parse_tool_entries(name, tools, self._max_tools_per_server) @@ -1615,7 +1705,7 @@ def _index_tools( ) for tool_id, tool_info in entries: self._tools[tool_id] = tool_info - indexed = len(entries) + indexed = _distinct_indexed(name, "tool", entries) if indexed and not self._catalog_publishing_suppressed(name): self._catalog_events.note_tools_changed() return indexed @@ -1632,12 +1722,13 @@ def _index_resources( Per-entry, for the reason spelled out on `_index_tools`, and `parsed` for the reason spelled out there too. The count returned is what was actually indexed, not what was offered, so a caller reporting it is not - overstating the catalog. + overstating the catalog -- a promise this docstring made before the + code kept it, and `_distinct_indexed` is where it is now kept. """ entries = _parse_resource_entries(name, resources) if parsed is None else parsed for resource_id, resource_info in entries: self._resources[resource_id] = resource_info - indexed = len(entries) + indexed = _distinct_indexed(name, "resource", entries) if indexed and not self._catalog_publishing_suppressed(name): self._catalog_events.note_resources_changed() return indexed @@ -1652,12 +1743,13 @@ def _index_prompts( """Index one server's prompts, skipping any entry we cannot parse. Per-entry, for the reason spelled out on `_index_tools`. The count - returned is what was actually indexed, not what was offered. + returned is what was actually indexed, not what was offered -- see + `_distinct_indexed`, which is where that is made true. """ entries = _parse_prompt_entries(name, prompts) if parsed is None else parsed for prompt_id, prompt_info in entries: self._prompts[prompt_id] = prompt_info - indexed = len(entries) + indexed = _distinct_indexed(name, "prompt", entries) if indexed and not self._catalog_publishing_suppressed(name): self._catalog_events.note_prompts_changed() return indexed @@ -2074,11 +2166,28 @@ async def _reconcile_once(self, name: str, managed: ManagedClient) -> None: usable[kind] = False continue if offered and not parsed: - logger.warning( - f"[{name}] Every {kind} entry in the listing was unparseable " - f"({len(offered)} offered); keeping the previous {kind} " - "rather than reporting them removed" - ) + if kind == "tools" and self._max_tools_per_server < 1: + # #175 item 3. A zero limit empties the parse result before + # a single entry is looked at, so the generic message below + # would accuse the downstream of sending garbage for a + # decision this gateway's own policy made. Deliberately no + # `Field(ge=1)` on the schema instead: `_load_policy(..., + # fatal=False)` swallows validation errors and falls back to + # the allow-all default, so rejecting the value would + # silently discard the operator's entire policy file (#202). + logger.warning( + f"[{name}] max_tools_per_server is " + f"{self._max_tools_per_server}, so none of the " + f"{len(offered)} tools offered could be indexed; " + "keeping the previous tools rather than reporting " + "them removed" + ) + else: + logger.warning( + f"[{name}] Every {kind} entry in the listing was " + f"unparseable ({len(offered)} offered); keeping the " + f"previous {kind} rather than reporting them removed" + ) usable[kind] = False continue usable[kind] = True @@ -3161,6 +3270,26 @@ async def adopt_process( logger.info(f"Adopting process for MCP server: {name}") + # #175 item 2. Every other path into the indexers clears this server's + # entries first. The two this one is modelled on are the connect and + # cleanup paths -- `_connect_stdio`, `_connect_remote_stream` and + # `_cleanup_client` -- which remove unconditionally, up front, exactly + # as here. `_reconcile_once` also removes before it indexes, but it is + # not the precedent for this line and should not be read as one: it + # fetches first and removes inside a synchronous apply block, per kind, + # only for the kinds whose re-listing actually succeeded. That is a + # different rule for a different situation (a live server whose prior + # catalog must survive a failed listing), and copying it here would be + # wrong. + # + # Without this, adopting a server that had been indexed under the same + # name left the previous listing's entries in the catalog beside the + # new one: tools the adopted process does not serve, still routable, + # until something else removed them. Uniform beats an exception + # documented in two places. Deliberately no line numbers -- the ones + # this comment first carried were stale within a single change. + self._remove_server_indexes(name) + # Initialize status status = ServerStatus( name=name, diff --git a/src/pmcp/types.py b/src/pmcp/types.py index acdf2158..0a56b974 100644 --- a/src/pmcp/types.py +++ b/src/pmcp/types.py @@ -955,7 +955,20 @@ class PromptPolicy(BaseModel): class LimitsPolicy(BaseModel): - """Resource limits policy.""" + """Resource limits policy. + + **`max_tools_per_server` deliberately carries no `Field(ge=1)`.** A zero + limit indexes no tools, which #175 item 3 first proposed to reject at the + schema. It must not be: `PolicyManager._load_policy(..., fatal=False)` -- + the auto-discovery path -- swallows any validation exception and leaves the + default allow-all `GatewayPolicy` in place, so a bound here would make a + policy file containing `max_tools_per_server: 0` schema-invalid and silently + discard **the whole file** -- allowlist, denylist, limits and redaction all + reverting to permissive. A cosmetic log fix is not worth a fail-open. The + misleading log was fixed in `_reconcile_once` instead; the underlying + fail-open is #202, and any future tightening of this schema carries the same + risk until it is resolved. + """ max_tools_per_server: int = 100 max_output_bytes: int = 50000 # 50KB diff --git a/tests/mcp2x/test_catalog_publishers.py b/tests/mcp2x/test_catalog_publishers.py index 703f57b0..8d65a0c6 100644 --- a/tests/mcp2x/test_catalog_publishers.py +++ b/tests/mcp2x/test_catalog_publishers.py @@ -127,7 +127,7 @@ def test_client_manager_with_no_catalog_events_constructs_with_a_null_sink() -> # And every existing call still works: a no-op index on a fresh manager # neither raises nor is a `_NullCatalogEventSink` special case. assert manager._index_tools("s", []) == 0 - assert manager._index_tools("s", [{"name": "t1"}]) == 1 + assert manager._index_tools("s", [{"name": "t1", "inputSchema": {}}]) == 1 # --- _index_tools / _index_resources / _index_prompts ---------------------- @@ -137,7 +137,9 @@ def test_index_tools_records_exactly_one_note_for_one_tool() -> None: sink = _RecordingSink() manager = ClientManager(catalog_events=sink) - indexed = manager._index_tools("s", [{"name": "t1", "description": "d"}]) + indexed = manager._index_tools( + "s", [{"name": "t1", "description": "d", "inputSchema": {}}] + ) assert indexed == 1 assert sink.calls == ["tools"] @@ -190,7 +192,7 @@ def test_index_resources_and_prompts_with_nothing_indexed_record_nothing() -> No def test_remove_server_indexes_records_all_three_kinds_that_were_present() -> None: sink = _RecordingSink() manager = ClientManager(catalog_events=sink) - manager._index_tools("s", [{"name": "t1"}]) + manager._index_tools("s", [{"name": "t1", "inputSchema": {}}]) manager._index_resources("s", [{"uri": "file:///a", "name": "r1"}]) manager._index_prompts("s", [{"name": "p1"}]) sink.calls.clear() @@ -215,7 +217,7 @@ def test_remove_server_indexes_only_notes_kinds_that_actually_shrank() -> None: resources or prompts changed too.""" sink = _RecordingSink() manager = ClientManager(catalog_events=sink) - manager._index_tools("s", [{"name": "t1"}]) + manager._index_tools("s", [{"name": "t1", "inputSchema": {}}]) sink.calls.clear() manager._remove_server_indexes("s") @@ -275,7 +277,7 @@ async def test_refresh_with_empty_config_publishes_all_three_list_changed_events published: list[ServerEvent] = [] bus.subscribe(published.append) - manager._index_tools("s", [{"name": "t1"}]) + manager._index_tools("s", [{"name": "t1", "inputSchema": {}}]) manager._index_resources("s", [{"uri": "file:///a", "name": "r1"}]) manager._index_prompts("s", [{"name": "p1"}]) await sink.flush() @@ -307,7 +309,7 @@ async def test_note_during_a_suspended_publish_via_index_methods_is_not_stranded sink = BusCatalogEventSink(bus) manager = ClientManager(catalog_events=sink) - manager._index_tools("s", [{"name": "t1"}]) + manager._index_tools("s", [{"name": "t1", "inputSchema": {}}]) await bus.entered.wait() # the drain is now suspended inside publish() manager._index_prompts("s", [{"name": "p1"}]) # lands in the lost-wakeup window diff --git a/tests/test_client_manager.py b/tests/test_client_manager.py index f83131bf..ed80b9a2 100644 --- a/tests/test_client_manager.py +++ b/tests/test_client_manager.py @@ -29,12 +29,14 @@ _infer_risk_hint, _remote_headers, _required_identity, + _required_object, _terminate_process_tree, _truncate_description, ) from pmcp.env_store import write_env_file from pmcp.remote_auth import MissingRemoteHeaderAuthError from pmcp.types import ( + LimitsPolicy, LocalMcpServerConfig, McpTaskInfo, McpTaskRecord, @@ -5878,3 +5880,503 @@ def test_required_identity_rejects_every_non_identity(self) -> None: for entry in ("a string", 3, None, ["a"]): with pytest.raises(TypeError): _required_identity(entry, "name") + + +class TestToolLimitIsEnforced: + """#175 item 1: the truncation boundary itself, pinned. + + Nothing in this file asserted how many tools land when a server offers more + than `max_tools_per_server`, and it showed: mutating the guard + `len(entries) >= limit` to `> limit` was the sole survivor of nine mutants + run against this file. That off-by-one lets a server put `limit + 1` tools + in the catalog -- the guard is a resource bound, so one more than the bound + every time is exactly the kind of drift a bound exists to stop. + + The assertions are therefore *exact* counts at `limit - 1`, `limit` and + `limit + 1`. "Fewer than offered" would pass under the mutant at + `limit + 1`, which is the only case that distinguishes `>=` from `>`. + """ + + LIMIT = 5 + + @staticmethod + def _listing(count: int) -> list[dict[str, Any]]: + """`inputSchema` on every entry: #175 item 4 makes a tool without one + unparseable, and a fixture that omitted it would report zero indexed + here for a reason that has nothing to do with the limit.""" + return [ + { + "name": f"t{i}", + "description": f"tool {i}", + "inputSchema": {"type": "object"}, + } + for i in range(count) + ] + + @pytest.mark.parametrize("offered", [LIMIT - 1, LIMIT, LIMIT + 1]) + def test_no_more_than_the_limit_is_ever_indexed(self, offered: int) -> None: + manager = ClientManager(max_tools_per_server=self.LIMIT) + + indexed = manager._index_tools("srv", self._listing(offered)) + + expected = min(offered, self.LIMIT) + assert indexed == expected + assert len(manager._tools) == expected + assert set(manager._tools) == {f"srv::t{i}" for i in range(expected)} + + def test_truncation_keeps_the_first_entries_and_says_so( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The entries kept are the leading ones, and the drop is not silent -- + a bound that discards tools without a word is indistinguishable from a + downstream that never offered them.""" + manager = ClientManager(max_tools_per_server=self.LIMIT) + + with caplog.at_level("WARNING", logger="pmcp.client.manager"): + indexed = manager._index_tools("srv", self._listing(self.LIMIT + 3)) + + assert indexed == self.LIMIT + assert set(manager._tools) == {f"srv::t{i}" for i in range(self.LIMIT)} + assert "srv::t5" not in manager._tools + assert any("truncating" in record.message for record in caplog.records) + + +class TestDuplicateIdentitiesAreNotDoubleCounted: + """#175 item 5: the count is of entries that landed, not entries offered. + + `_index_resources`' docstring promised exactly this -- "the count returned + is what was actually indexed, not what was offered, so a caller reporting + it is not overstating the catalog" -- while the code returned + `len(entries)`. Two entries sharing an identity are two list items and one + catalog key, so the promise was false precisely when identities collide, + and a documented guarantee contradicted by the code is worse than an + undocumented gap. + + Each kind is asserted against **its own** catalog: `_index_tools` against + `_tools`, `_index_resources` against `_resources`, `_index_prompts` against + `_prompts`. Comparing all three to `_tools` would prove nothing for the + other two. + """ + + def test_duplicate_tools_are_counted_once_and_the_last_one_wins(self) -> None: + manager = ClientManager() + + indexed = manager._index_tools( + "srv", + [ + {"name": "dup", "description": "first", "inputSchema": {}}, + {"name": "dup", "description": "second", "inputSchema": {}}, + {"name": "solo", "description": "only", "inputSchema": {}}, + ], + ) + + assert set(manager._tools) == {"srv::dup", "srv::solo"} + assert indexed == len(manager._tools) == 2 + assert manager._tools["srv::dup"].description == "second" + + def test_duplicate_resources_are_counted_once(self) -> None: + manager = ClientManager() + + indexed = manager._index_resources( + "srv", + [ + {"uri": "mem://dup", "name": "first"}, + {"uri": "mem://dup", "name": "second"}, + {"uri": "mem://solo"}, + ], + ) + + assert set(manager._resources) == {"srv::mem://dup", "srv::mem://solo"} + assert indexed == len(manager._resources) == 2 + assert manager._resources["srv::mem://dup"].name == "second" + + def test_duplicate_prompts_are_counted_once(self) -> None: + manager = ClientManager() + + indexed = manager._index_prompts( + "srv", + [ + {"name": "dup", "title": "first"}, + {"name": "dup", "title": "second"}, + {"name": "solo"}, + ], + ) + + assert set(manager._prompts) == {"srv::dup", "srv::solo"} + assert indexed == len(manager._prompts) == 2 + assert manager._prompts["srv::dup"].title == "second" + + def test_a_listing_that_is_entirely_duplicates_counts_one(self) -> None: + """The edge the plan calls out. One key lands, so the count is one -- + and it is emphatically not zero, which would route the listing into + `_reconcile_once`'s offered-but-none-parseable rule and preserve a + stale catalog on the strength of a duplicate.""" + manager = ClientManager() + + indexed = manager._index_tools( + "srv", + [ + {"name": "same", "inputSchema": {}}, + {"name": "same", "inputSchema": {}}, + {"name": "same", "inputSchema": {}}, + ], + ) + + assert indexed == len(manager._tools) == 1 + + def test_the_collision_is_logged_so_it_is_diagnosable( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The honest count makes last-write-wins visible; the log is what + makes it diagnosable. Logged at DEBUG, naming the colliding id.""" + manager = ClientManager() + + with caplog.at_level("DEBUG", logger="pmcp.client.manager"): + manager._index_tools( + "srv", + [ + {"name": "dup", "inputSchema": {}}, + {"name": "dup", "inputSchema": {}}, + ], + ) + + assert any( + "srv::dup" in record.message and record.levelname == "DEBUG" + for record in caplog.records + ) + + def test_duplicates_publish_once_not_twice(self) -> None: + """The publish gate keys off the count, so it must not have moved: a + listing that indexed something still publishes exactly one note.""" + sink = _RecordingSink() + manager = ClientManager(catalog_events=cast(Any, sink)) + + manager._index_tools( + "srv", + [ + {"name": "dup", "inputSchema": {}}, + {"name": "dup", "inputSchema": {}}, + ], + ) + + assert (sink.tools, sink.resources, sink.prompts) == (1, 0, 0) + + +class TestZeroLimitLogsAccurately: + """#175 item 3: `max_tools_per_server: 0` must not be reported as a + downstream that sent garbage. + + A zero limit empties `_parse_tool_entries`' result before a single entry is + examined, so `_reconcile_once`'s offered-but-none-parseable branch fired + and announced "Every tools entry in the listing was unparseable" -- an + accusation aimed at the server for a decision this gateway's own policy + file made. The operator's next move is to fix the policy, and the log has + to point there. + + Deliberately fixed in the *log* and not in the schema. `LimitsPolicy` gets + no `Field(ge=1)`: `PolicyManager._load_policy(..., fatal=False)` -- the + auto-discovery path -- swallows any validation exception and leaves the + default allow-all `GatewayPolicy` in place, so making `0` schema-invalid + would silently discard the operator's *entire* policy file, allow and deny + lists and redaction included. That fail-open is #202 and is not this + change's to fix; the test below pins that `0` still validates, so a future + tightening cannot land here unnoticed. + """ + + @staticmethod + def _managed(name: str) -> ManagedClient: + status = ServerStatus(name=name, status=ServerStatusEnum.ONLINE, tool_count=0) + managed = ManagedClient(config=MagicMock(), process=MagicMock(), status=status) + managed.config.name = name + return managed + + @staticmethod + def _wire(manager: ClientManager, tools: list[dict[str, Any]]) -> None: + async def fake_send( + managed: ManagedClient, + method: str, + params: dict[str, Any], + *args: Any, + **kwargs: Any, + ) -> dict[str, Any]: + kind = method.split("/")[0] + return {kind: list(tools) if kind == "tools" else []} + + manager._send_request = fake_send # type: ignore[method-assign] + + def test_a_zero_limit_still_validates_on_the_policy_schema(self) -> None: + """Rejecting it is what triggers #202's fail-open, so it must not be + rejected.""" + assert LimitsPolicy(max_tools_per_server=0).max_tools_per_server == 0 + + @pytest.mark.asyncio + async def test_zero_limit_names_the_limit_and_blames_no_one( + self, caplog: pytest.LogCaptureFixture + ) -> None: + manager = ClientManager(max_tools_per_server=0) + managed = self._managed("srv") + manager._clients["srv"] = managed + self._wire(manager, [{"name": "alpha", "inputSchema": {}}]) + + with caplog.at_level("WARNING", logger="pmcp.client.manager"): + manager._handle_downstream_notification( + "srv", managed, "notifications/tools/list_changed" + ) + + async def _wait() -> None: + while manager._reconcile_tasks: + await asyncio.sleep(0.005) + + await asyncio.wait_for(_wait(), 5.0) + + messages = [record.message for record in caplog.records] + assert any("max_tools_per_server is 0" in message for message in messages), ( + f"no message named the zero limit: {messages}" + ) + assert not any("unparseable" in message for message in messages), ( + f"a zero limit was reported as a malformed listing: {messages}" + ) + + def test_a_positive_limit_still_reports_truncation_the_old_way( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The zero-limit wording is a special case, not a replacement: an + ordinary overrun must still say it truncated.""" + manager = ClientManager(max_tools_per_server=1) + + with caplog.at_level("WARNING", logger="pmcp.client.manager"): + manager._index_tools( + "srv", + [ + {"name": "a", "inputSchema": {}}, + {"name": "b", "inputSchema": {}}, + ], + ) + + assert any("truncating" in record.message for record in caplog.records) + + @pytest.mark.asyncio + async def test_a_genuinely_unparseable_listing_still_says_unparseable( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The other half of the same guarantee: the accusation is still made + when it is true.""" + manager = ClientManager(max_tools_per_server=100) + managed = self._managed("srv") + manager._clients["srv"] = managed + self._wire(manager, [{"name": ""}, {"nope": True}]) + + with caplog.at_level("WARNING", logger="pmcp.client.manager"): + manager._handle_downstream_notification( + "srv", managed, "notifications/tools/list_changed" + ) + + async def _wait() -> None: + while manager._reconcile_tasks: + await asyncio.sleep(0.005) + + await asyncio.wait_for(_wait(), 5.0) + + messages = [record.message for record in caplog.records] + assert any("unparseable" in message for message in messages), messages + + +class TestAdoptProcessRemovesStaleIndexesFirst: + """#175 item 2: `adopt_process` must clear the server's catalog entries + before it indexes, like every other path into the indexers. + + `_connect_stdio`, `_reconcile_once` and `_cleanup_client` all remove this + server's entries first. `adopt_process` did not, so adopting a server that + had already been indexed under the same name left the previous listing's + tools in the catalog alongside the new ones -- entries the adopted process + does not serve, still routable, until something unrelated removed them. + + Asserted on catalog *contents*, not on whether `_remove_server_indexes` was + called: a test that only checks the call passes just as happily if the + removal runs after the index. + """ + + @staticmethod + def _process() -> Any: + process = MagicMock() + process.returncode = None + process.stdin = MagicMock() + process.stdout = MagicMock() + process.stderr = None + return process + + @staticmethod + def _config(name: str) -> Any: + config = MagicMock() + config.name = name + return config + + async def _adopt( + self, manager: ClientManager, name: str, tools: list[dict[str, Any]] + ) -> None: + async def fake_listings(managed: ManagedClient) -> dict[str, Any]: + return {"tools": list(tools), "resources": [], "prompts": []} + + with ( + patch.object(manager, "_read_stdout", new=AsyncMock(return_value=None)), + patch.object(manager, "_read_stderr", new=AsyncMock(return_value=None)), + patch.object(manager, "_send_initialize", new=AsyncMock(return_value=None)), + patch.object(manager, "_fetch_server_listings", new=fake_listings), + ): + await manager.adopt_process(name, self._process(), self._config(name)) + + @pytest.mark.asyncio + async def test_a_prior_listings_tools_do_not_survive_the_adopt(self) -> None: + manager = ClientManager() + manager._index_tools("srv", [{"name": "stale", "inputSchema": {}}]) + assert set(manager._tools) == {"srv::stale"} + + await self._adopt(manager, "srv", [{"name": "fresh", "inputSchema": {}}]) + + assert set(manager._tools) == {"srv::fresh"}, ( + "adopt_process indexed on top of the previous catalog" + ) + assert manager._servers["srv"].tool_count == 1 + + @pytest.mark.asyncio + async def test_resources_and_prompts_are_cleared_too(self) -> None: + """The removal is per server, not per kind -- an adopt that replaced + only the tools would leave a stale resource just as routable.""" + manager = ClientManager() + manager._index_resources("srv", [{"uri": "mem://stale"}]) + manager._index_prompts("srv", [{"name": "stale"}]) + + await self._adopt(manager, "srv", [{"name": "fresh", "inputSchema": {}}]) + + assert manager._resources == {} + assert manager._prompts == {} + + @pytest.mark.asyncio + async def test_another_servers_entries_are_untouched(self) -> None: + """`_remove_server_indexes` is per-server-name, and the adopt must not + widen that: clearing the catalog wholesale would be a much worse bug + than the one being fixed.""" + manager = ClientManager() + manager._index_tools("other", [{"name": "keep", "inputSchema": {}}]) + + await self._adopt(manager, "srv", [{"name": "fresh", "inputSchema": {}}]) + + assert set(manager._tools) == {"other::keep", "srv::fresh"} + + @pytest.mark.asyncio + async def test_adopting_a_name_with_no_prior_index_is_unchanged(self) -> None: + """The edge the plan calls out: removing nothing must not be an error + and must not stop the fresh listing being indexed.""" + manager = ClientManager() + + await self._adopt(manager, "srv", [{"name": "fresh", "inputSchema": {}}]) + + assert set(manager._tools) == {"srv::fresh"} + + +class TestMissingInputSchemaIsUnparseable: + """#175 item 4: a tool that declares no `inputSchema` is not indexed. + + `tool.get("inputSchema", {})` manufactured a schema the server never sent, + and `{}` does not mean "we do not know" -- it means "any arguments at all + are valid", which is then published to every caller and every model reading + the catalog. MCP requires `inputSchema` on a tool, so a tool without one is + a tool we could not read, and #172 already settled what happens to those: + skip it and say so, rather than invent the missing value. + + This is the only behaviour change in #175 and is called out as such in the + changelog: a downstream that omitted `inputSchema` used to be accepted with + a permissive schema and is now skipped. + """ + + def test_a_tool_without_an_input_schema_is_skipped_and_logged( + self, caplog: pytest.LogCaptureFixture + ) -> None: + manager = ClientManager() + + with caplog.at_level("WARNING", logger="pmcp.client.manager"): + indexed = manager._index_tools("srv", [{"name": "schemaless"}]) + + assert indexed == 0 + assert manager._tools == {} + assert any( + "unparseable tool" in record.message and "schemaless" in record.message + for record in caplog.records + ), [record.message for record in caplog.records] + + def test_a_tool_with_an_input_schema_is_indexed_unchanged(self) -> None: + """The other half: nothing about a well-formed tool moved.""" + manager = ClientManager() + schema = {"type": "object", "properties": {"q": {"type": "string"}}} + + indexed = manager._index_tools( + "srv", [{"name": "ok", "description": "d", "inputSchema": schema}] + ) + + tool = manager.get_tool("srv::ok") + assert indexed == 1 + assert tool is not None + assert tool.input_schema == schema + + def test_an_explicitly_empty_schema_is_still_an_answer(self) -> None: + """`{}` is the server saying "any arguments", which is a thing it is + entitled to say. Only the *absence* is unreadable -- rejecting `{}` + would drop tools that are behaving correctly.""" + manager = ClientManager() + + assert manager._index_tools("srv", [{"name": "any", "inputSchema": {}}]) == 1 + assert set(manager._tools) == {"srv::any"} + + @pytest.mark.parametrize( + "schema", [None, "not-a-dict", ["not", "a", "dict"], 3, True] + ) + def test_a_non_object_schema_is_unparseable_too(self, schema: Any) -> None: + """`inputSchema: null` is distinct from absent and just as unusable; + so is any other non-object. Before this, `null` reached + `_schema_dialect` and only failed there by accident of a `.get` call.""" + manager = ClientManager() + + assert ( + manager._index_tools("srv", [{"name": "bad", "inputSchema": schema}]) == 0 + ) + assert manager._tools == {} + + def test_a_schemaless_tool_costs_only_itself(self) -> None: + """Per-entry, like every other parse failure: the tools either side of + it are still indexed.""" + manager = ClientManager() + + indexed = manager._index_tools( + "srv", + [ + {"name": "before", "inputSchema": {}}, + {"name": "schemaless"}, + {"name": "after", "inputSchema": {}}, + ], + ) + + assert indexed == 2 + assert set(manager._tools) == {"srv::before", "srv::after"} + + def test_required_object_rejects_every_non_object(self) -> None: + """The helper directly, so the rule is pinned independently of the + parser that calls it -- and so the next reader can see why it is not + `_required_identity`, which requires a non-empty *string* and would + reject every valid tool.""" + assert _required_object({"inputSchema": {}}, "inputSchema") == {} + assert _required_object({"inputSchema": {"type": "object"}}, "inputSchema") == { + "type": "object" + } + for entry in ( + {}, + {"inputSchema": None}, + {"inputSchema": ""}, + {"inputSchema": "{}"}, + {"inputSchema": []}, + {"inputSchema": 0}, + ): + with pytest.raises((TypeError, ValueError)): + _required_object(entry, "inputSchema") + for entry in ("a string", 3, None, ["a"]): + with pytest.raises(TypeError): + _required_object(entry, "inputSchema")