fix(catalog): close the five catalog-indexing findings (#175)

.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.

.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