Repository navigation
Adjacent catalog-indexing findings split out of #173 #175
Description
Split out of #173 so that issue closes cleanly on its actual subject (catalog listing pagination, fixed in #174).
These five surfaced during the advisor-board rounds on #172 and #174. All are pre-existing and none is blocking — they are recorded here so they stay visible rather than disappearing with a closed issue.
1. max_tools_per_server truncation is untested behaviourally
A mutation of the truncation boundary (len(entries) >= limit → >) survived the full tests/test_client_manager.py. Only the limit's configurability is covered (tests/test_baseline_constraints.py:504); nothing asserts its effect. Found by a mutation battery, and it was the sole survivor of nine.
A test should index more entries than the limit and assert exactly limit land in the catalog.
2. adopt_process indexes without prior removal
ClientManager.adopt_process (manager.py, ~:2977) indexes a server's capabilities with no preceding _remove_server_indexes. Every other path that indexes — _connect_stdio, _connect_sse, _reconcile_once — removes first.
This matters mainly because the reconciliation docstrings justify per-entry connect-time behaviour with "connect removes the server's indexes first", which is true of the connect paths but not of adopt_process. Either the path should remove first for consistency, or the docstring should name the exception. Install-time and low stakes, but the reasoning currently reads as covering a case it does not.
3. LimitsPolicy.max_tools_per_server has no lower bound
types.py:953 accepts 0. At 0, every non-empty listing classifies as "all entries unparseable" and logs a message saying exactly that — which is untrue; the entries were fine, the limit was zero.
Behaviourally inert today (nothing indexes in such a session either way), but the log actively misleads. Field(ge=1) would prevent it.
4. inputSchema is defaulted rather than required
tool.get("inputSchema", {}) manufactures an accept-anything schema when a required field is missing. This is the same manufacture-a-value pattern as the identity defaults fixed in #172 — resource.get("uri", "") producing the identity srv:: — at lower stakes, since there is no identity collapse.
Worth deciding deliberately: either a missing inputSchema makes the entry unparseable (consistent with how a missing identity is now treated), or the default is intentional and should say so.
5. Duplicate identities within one listing collapse silently
Two entries sharing an identity overwrite each other in the catalog dict (last write wins) while the indexed count counts both, so reported counts can overstate what is actually in the catalog. No incorrect behaviour follows today, but the count is not the truth.
Suggested handling
(1) and (3) are small and self-contained. (2) is a one-line consistency fix or a docstring amendment. (4) and (5) are decisions before they are changes — they should be settled deliberately rather than fixed by reflex, since both currently-wrong-looking behaviours may be intentional.
Activity
docs(catalog): record the #175 mutation evidence in the repo
chore: re-trigger CI (no content change)
fix(catalog): close the five catalog-indexing findings (#175) (#204)
ViperJuice commentedon Aug 29, 2026
Closed by #204 (merged as
d3ddbd4). All five findings closed.1 — the tool-limit boundary is now pinned. The mutation that motivated this issue (
len(entries) >= limit→>) survived the entire existing test file and was the sole survivor of nine. Recorded in.consiliency/evidence/mutation-175.mdwith captured exit codes: unchanged 0, mutant 1 (assert 6 == 5), restored 0,git diffempty after each restore. The test pinslimit - 1,limit, andlimit + 1.2 —
adopt_processnow removes before indexing, matching connect and cleanup. Asserted on catalog contents, not on a call count; the removal-line mutant is recorded the same way (0 → 1 → 0).3 — deliberately adds no schema bound. The first draft added
Field(ge=1)tomax_tools_per_server; that was caught as a security regression and removed.PolicyManager._load_policy(..., fatal=False)swallows validation errors and falls back to the default allow-all policy, so making0invalid would have silently discarded an operator's entire policy file — allow/deny lists, limits and redaction. Only the misleading log is fixed: a zero limit now says so instead of blaming the server for "all entries unparseable". A test pins thatLimitsPolicy(max_tools_per_server=0)still validates, and the docstring records why. The underlying fail-open is #202.4 — a tool without
inputSchemais now unparseable, skipped and logged like any other bad entry, rather than being handed a manufactured accept-anything{}. Same ruling #172 made for identities. This is a behaviour change and the CHANGELOG says so under### Changed. Implemented with a required-object check, not_required_identity— that helper requires a non-empty string and would have rejected every valid tool.5 — the count is now what landed.
entriesis a list, the loop assigns into a dict, so duplicate identities overwrote whilelen(entries)counted both — contradicting a docstring that promised the count was not overstated. Each_index_*is now asserted against its own catalog.One regression was caused and caught during the work, recorded in the evidence file because no mutation of product code would have found it: the first cut of item 5 moved the catalog write into a helper taking the dict as a parameter. The publisher-coverage AST guard attributes writes to their enclosing method, so the refactor did not trip the guard — it silently emptied it. The write is back in the mutators; the guard requires all five publishing methods to appear as direct writers, verified independently.
docs(policy): reconcile the #175 note and state the JSON/YAML asymmet…
fix(policy): an auto-discovered policy that parses must not fail open (…
chore: v2.7.0 (#206)
fix(policy): bound max_tools_per_server now that #202 makes validatio…