diff --git a/.consiliency/plans/detailed-installed-version-from-serverinfo-20260817-0400.md b/.consiliency/plans/detailed-installed-version-from-serverinfo-20260817-0400.md new file mode 100644 index 00000000..6d630b1a --- /dev/null +++ b/.consiliency/plans/detailed-installed-version-from-serverinfo-20260817-0400.md @@ -0,0 +1,187 @@ +# Detailed plan: probe the installed version directly from `initialize` + +## Task + +Fix Consiliency/pmcp#150 at its root, after four rejected attempts (PR #154 closed; the re-keying plan boarded 3/3 DISAGREE). + +The board's decisive finding: **nothing in pmcp ever observes an installed version.** `get_package_version` returns *upstream latest* only, and `GeneratedServerDescriptions.version` is itself an upstream snapshot taken at describe time. So the "update available" comparison is upstream-latest-at-describe-time vs upstream-latest-now, which is only meaningful while the described package and the configured package are the same — the exact assumption #150 breaks. + +Maintainer decision: **probe the installed version directly**, rather than continuing to infer it. + +## Research summary + +The probe already exists in the protocol and pmcp is discarding it. + +`_send_initialize` (`client/manager.py`) reads the `initialize` result and stores `protocolVersion` into `managed.status.protocol_version` and `capabilities` into `managed.status.server_capabilities` — then ignores the rest of the same dict. That dict contains `serverInfo`. + +Verified against the installed `mcp` library: + +``` +InitializeResult fields: ['meta', 'protocol_version', 'capabilities', 'server_info', 'instructions'] +Implementation fields: ['name', 'title', 'version', 'description', 'website_url', 'icons'] +InitializeResult.server_info required: True +``` + +`serverInfo` is a **required** field carrying `name` and `version`. Every conforming server reports its own version on every connect, and pmcp throws it away. That is the installed-version source the previous four attempts lacked, and it needs no new subprocess, no new network call, and no new failure mode: it is a field already present in a handshake pmcp already performs and already parses. + +Two consequences that shape the design: + +- **The value is authoritative for what is *running*, not what is on disk.** It is reported by the live process, so it answers exactly the question the notice asks ("is the thing serving me out of date?") and is immune to the config-drift class of bug that #150/#151 are about — a restarted server re-reports. +- **It is self-declared and free-form.** The spec does not constrain the string to semver, and a server may report something `is_version_newer` cannot parse. The design must degrade to *silence*, never to a fabricated comparison. + +`ServerStatus` (`types.py:429`) is the natural home: it already holds `protocol_version` and `server_capabilities` from the same handshake. + +## Changes + +### `src/pmcp/types.py` (modify) + +- `ServerStatus` — **modify** — add `server_version: str | None = None` (and `server_title: str | None = None` if trivially available), beside the existing `protocol_version`/`server_capabilities` from the same handshake. Defaulted, so every existing construction site is unaffected. + +### `src/pmcp/client/manager.py` (modify) + +- `_send_initialize` — **modify** — after the existing `capabilities` block, read `result.get("serverInfo")` and store `version` (as `str`, when present and non-empty) into `managed.status.server_version`. Defensive: `serverInfo` is spec-required but this is untrusted downstream input, so a missing/malformed value leaves the field `None` rather than raising. This is the only place the value is captured; it refreshes automatically on every connect and reconnect. + +### `src/pmcp/tools/handlers.py` (modify) + +- `_installed_version(server_name)` — **add** — returns the live `server_version` for an ONLINE server via the client manager, else `None`. Single accessor so no caller re-derives it. +- `_get_update_warning` — **modify** — take `current` from `_installed_version(...)` when the server is online. Falls back to today's `descriptions_cache.version` **only** when the server is offline *and* the cache entry's `package` matches the effective config's package (the check added in #154, which was accepted in every round). Otherwise return `None`. +- `_run_stale_index` — **modify** — same source for `current`, same fallback rule. +- `catalog_search` `stale_updates` (`~1698`) — **modify** — the reader that leaked in all three prior rounds. It must apply the same TTL as the other two readers, which it currently does not. Extract the emit predicate so all three readers provably share one contract. +- `_stale_check_cache` — **modify** — store the version *source* alongside the tuple (`"installed" | "described"`), so a described-fallback entry can never be mistaken for an observed one by a later reader. +- `_notice_package_matches_cache` — **keep**, now only guarding the offline fallback path. +- `_repoint_cache_entry_to_effective_package` — **delete** — this was the #154 "self-heal" that set `version=""` and produced permanent silence, and that erased a good baseline written by `update_server`. With a real installed version there is nothing to re-point. +- `_effective_notice_target` — **keep** unchanged (precedence + `is_server_allowed`; accepted in every round). +- `update_server` drift recheck — **keep** unchanged (the #151 fix: recheck after probe *and* after version lookup, compared via `_refresh_config_unchanged`). + +### `tests/test_tools.py` (modify) + +Every test asserts an **observable notice outcome**. Three rounds were passed by tests asserting a queried argument or a mutated label while the bug survived; that shape is banned. + +- **add** — `test_notice_uses_installed_version_from_server_info`: server reports `1.0.0` via `serverInfo`, upstream latest is `2.0.0` → a notice is emitted naming `1.0.0 -> 2.0.0`. +- **add** — `test_no_notice_when_installed_equals_latest`: reports `2.0.0`, latest `2.0.0` → silence. +- **add** — `test_package_change_does_not_fabricate_a_notice` (**the #150 case**): config switches A→B, server reports B's real version → the comparison is B-vs-B, and either a correct B notice or silence is emitted — never an A-vs-B comparison. +- **add** — `test_notice_resumes_after_package_change`: the criterion the permanent-silence bug failed. After an A→B switch, a real notice for B **is** emitted once the server reports its version. Asserts the emitted string, not a field. +- **add** — `test_unparseable_server_version_stays_silent`: server reports `"nightly-2026-08-17"` → no notice, no exception. +- **add** — `test_missing_server_info_stays_silent`: server omits `serverInfo` → no notice. +- **add** — `test_offline_fallback_requires_package_match`: offline server, cache describes A, config names B → silence. +- **add** — `test_catalog_search_applies_the_same_ttl`: expired entry is not emitted. +- **keep** — accepted #151 tests and the accepted #150 precedence/policy/remote tests. + +### `tests/test_client_manager.py` (modify — or the nearest existing manager test module) + +- **add** — `test_initialize_captures_server_version`: a handshake reporting `serverInfo.version` populates `ServerStatus.server_version`; a handshake omitting it leaves `None`. + +## Documentation impact + +- `CHANGELOG.md` — **modify** — entry under `## [Unreleased]`. **Mandatory**: `main` is protected and the `changelog` check fails any PR touching `src/` without it. Must state plainly that update notices now compare the **running server's reported version** against upstream latest, and that a server which does not report a usable version produces no notice. +- `README.md` — **modify if** it documents `gateway.health`/`describe` output shape, since `ServerStatus` gains a public field. Check before writing; if the field is not surfaced in documented output, state `no doc footprint` instead. + +## Frozen vocabulary / protocol check + +`ServerStatus` is a **public output model** (surfaced by `gateway.health`), so adding `server_version` is an additive schema change: new optional field, no field renamed, removed, or retyped. `serverInfo` is read from the existing `initialize` handshake — pmcp already parses `protocolVersion` and `capabilities` from the same result — so **no new protocol vocabulary is introduced** and no new request is sent. + +## Dependencies & order + +1. `ServerStatus.server_version` first — the capture site depends on the field existing. +2. Capture in `_send_initialize`, with its manager test. Independently verifiable before any notice logic changes. +3. `_installed_version` accessor, then migrate the three readers together — a partial migration leaves readers disagreeing about what `current` means, which is the failure mode of every prior round. +4. Delete `_repoint_cache_entry_to_effective_package`. +5. Tests, each proven RED. +6. CHANGELOG last. + +No external dependencies; no migration (all state is in-memory and rebuilt on connect). + +## Verification + +```bash +cd + +uv run pytest tests/ -q -k 'server_version or notice or stale or catalog_search or update_server' + +# RED proof — required. Revert ONLY the src changes via a targeted +# `git diff > patch` + `git apply -R`. NEVER `git checkout HEAD -- .` +# (it has destroyed uncommitted work in this repo). + +uv run pytest -q # baseline 2536 + new tests, 3 skipped, 0 failed +uv run ruff check . +uv run ruff format --check src/ tests/ +uv run mypy src +``` + +Then reproduce the round-3 defect by hand and confirm it is gone: drive `_get_update_warning` three consecutive times across an A→B config switch with the server reporting B's version, and confirm a correct notice appears (the failing transcript printed `None` three times). + +Edge cases: server online vs offline; `serverInfo` absent; version unparseable by `is_version_newer`; remote server (does it report `serverInfo`? — confirm, and if so this works identically); server never connected (lazy); policy-denied; package undetectable. + +## Acceptance criteria + +- [ ] `ServerStatus.server_version` is populated from the `initialize` handshake and left `None` when the server omits or malforms it — proven by `test_initialize_captures_server_version`. +- [ ] Update notices compare the **running server's reported version** against upstream latest — proven by `test_notice_uses_installed_version_from_server_info` and `test_no_notice_when_installed_equals_latest`. +- [ ] A `.mcp.json` override naming a different package than the manifest never produces a cross-package comparison — proven by `test_package_change_does_not_fabricate_a_notice`. +- [ ] Notices **resume** after a package change rather than stopping permanently — proven by `test_notice_resumes_after_package_change` asserting the emitted notice string. +- [ ] An unusable or absent reported version yields silence, never a fabricated or crashing comparison — proven by `test_unparseable_server_version_stays_silent` and `test_missing_server_info_stays_silent`. +- [ ] Full suite ≥2536 + new tests passed / 3 skipped / 0 failed; `ruff check`, `ruff format --check src/ tests/`, `mypy src` clean; `## [Unreleased]` CHANGELOG entry present. + +## Open question for the board + +The offline fallback keeps the old described-version comparison when a server is not running. That is strictly weaker evidence than an installed version and is the residue of the original bug class. Is retaining it worth it for offline coverage, or should an offline server simply produce no update notice at all — trading coverage for the guarantee that **every notice pmcp emits is backed by an observed installed version**? The simpler contract may be the more honest one. + +## Execution Policy + +- execute: effort=high, reason=four prior attempts rejected in this subsystem; touches the connect handshake plus three notice readers, and the failure mode is a silently wrong user-facing claim. + +## Automation + +```yaml +automation: + suite_command: "uv run pytest -q" +``` + +--- + +## Board review — REJECTED (3/3 DISAGREE). Do not implement as written. + +Boarded before implementation. All three legs blocked. Verified against source and reproduced; recorded so the next attempt starts from facts rather than from my framing. + +### B1 (fatal) — `serverInfo.version` is NOT an installed-package version + +The plan's premise. It is wrong, and **this repo already documented it** — `plans/phase-plan-v11-PG.md:44`, which I failed to find before writing the plan: + +> "1.x FastMCP defaulted it to the *mcp library* version (a probe against the current build reports `{"name": "pangram", "version": "1.29.0"}` — plainly wrong, it is not this package's version). `MCPServer.__init__` defaults `version: str = ""`, so a literal port reports an empty string." + +MCP defines `serverInfo.version` as an *implementation* version, unconstrained and self-declared. FastMCP 1.x reports the SDK version; mcp 2.x defaults it to `""`. Comparing that against an npm/PyPI registry version can fabricate or hide updates just as readily as the bug being fixed. + +My empirical check was not wrong, but it was not sufficient: context7 reports `4.0.2` and `npm view @upstash/context7-mcp version` is also `4.0.2`. One well-behaved server proves the value *can* match, not that it *must*. A single confirming example is not an identity guarantee, and I over-read it. + +### B2 — package identity is still unbound + +`_effective_notice_target` reloads current *disk* config, while `server_version` describes the *connected process*, whose connect-time config is separately available (`manager.py:2732`). After an unrefreshed edit, an online A process can still be compared against B's latest. Conversely, after reconnecting as B, the server-scoped stale cache can retain A's tuple — and the plan's `"installed" | "described"` marker records neither package nor connection identity, so it does not close this. + +### B3 — "unparseable version means silence" is false + +`is_version_newer` (`version_checker.py:436`) extracts digits and never reports a parse failure. Reproduced: + +``` +is_version_newer('nightly', '2.0.0') -> True +is_version_newer('release-channel-a', '2.0.0') -> True +is_version_newer('build-1', '2.0.0') -> True +is_version_newer('', '2.0.0') -> True # the mcp 2.x default! +is_version_newer('1.29.0', '2.0.0') -> True # the FastMCP SDK-version case +``` + +Each emits a notice. And my proposed test used `'nightly-2026-08-17'`, which returns `False` — it would have **passed by accident** while the criterion it claimed to prove was false. That is the fourth time a test of mine would have passed while its bug survived; the pattern is mine, not the board's. + +Note the two worst cases are exactly B1's: an empty version (mcp 2.x default) and an SDK version (FastMCP 1.x) both compare as "older" and would produce a fabricated notice for every such server. + +### What this means for #150 + +Direct probing is still the right instinct, but `serverInfo.version` is the wrong probe. Any fix needs a version whose **identity is bound to the package being compared**. Candidates, none yet assessed: + +- Query the package manager for the actually-installed version (`npm ls`, `uv pip show`, `cargo`, `docker image inspect`) — real identity, but a new subprocess per server and per-ecosystem handling. +- Record the version resolved at install/update time as provenance, and compare only that. +- Narrow the feature: only emit notices for servers pmcp installed itself, where provenance is known. + +Any of these needs `is_version_newer` to gain a fail-closed parse mode first (B3), or it will fabricate notices regardless of the source. + +### Status + +Plan rejected, not implemented. #151's fix remains independent and accepted. diff --git a/.consiliency/plans/detailed-stale-notice-package-identity-20260817-0300.md b/.consiliency/plans/detailed-stale-notice-package-identity-20260817-0300.md new file mode 100644 index 00000000..72299371 --- /dev/null +++ b/.consiliency/plans/detailed-stale-notice-package-identity-20260817-0300.md @@ -0,0 +1,158 @@ +# Detailed plan: key the stale-update caches on package identity + +## Task + +Fix Consiliency/pmcp#150 structurally, after three board rounds rejected three successive patch-level attempts (PR #154, closed). The narrow #151 fix from that PR is carried forward here unchanged in intent. + +The recurring defect: **the gateway compares a version recorded for one package against the upstream latest of a different package**, producing a fabricated "update available" notice or hiding a real one. Every fix so far has patched one reader while another kept serving the stale tuple. + +## Research summary + +Reconnaissance done inline (all files in session from PR #147 and #154). The structural findings, verified at PR #154 head `85918b1`: + +**Two caches carry a package identity neither is keyed on.** + +1. `self._stale_check_cache: dict[str, tuple[float, str | None, str | None]]` (`handlers.py:1088`) — keyed by **server name**, holding `(timestamp, current, latest)`. Nothing in the key or the tuple records *which package* those versions describe. +2. `self._descriptions_cache.servers[name]` — a `GeneratedServerDescriptions` carrying both `package` and `version`. It *does* record the package, but the version baseline is only ever written by `refresh_all` (manifest-only) and by `update_server` on success (`handlers.py:5394`). + +**Three readers, three different contracts** — this is why patching is not converging: + +| Reader | Line | TTL check | Identity check | +|---|---|---|---| +| `_run_stale_index` (background sweep) | `1347` | yes — **before** resolving the target | patched in #154 | +| `_get_update_warning` | `3790` | yes | patched in #154 | +| `catalog_search` `stale_updates` | `1698-1704` | **no** | **no** | + +`catalog_search` iterates raw tuples and emits a notice for anything where `is_version_newer(current, latest)`. It is the reader that kept leaking in every round. + +**Why the sweep's ordering defeats eviction.** `_run_stale_index` skips fresh entries (`1348`) *before* it resolves the effective target, so a poisoned-but-warm entry is never re-examined and never evicted — it simply survives its full 6h TTL (`1089`) while `catalog_search` serves it. + +**Why the #154 "self-heal" produced permanent silence.** Re-pointing set `package=B, version=""`; `_get_update_warning` returns early on an empty version (`3782-3787`) *before* any lookup, and the sweep skips fresh entries before resolving — so no path ever observes a version for B. Reproduced at exact head: three consecutive passes, three `None`. And `update_server` writing B's version (`5394`) is then erased by the next re-point, so a real update to B stays hidden. + +**Why "wait for `pmcp refresh`" is not a recovery.** `refresh_all` targets `servers or list(manifest.servers.keys())` (`refresher.py:335`) — manifest entries only. A server whose effective package comes from a `.mcp.json` override never regains a matching entry. + +## Changes + +### `src/pmcp/tools/handlers.py` (modify) + +- `_stale_check_cache` type + initialisation (`~1088`) — **modify** — re-key from `dict[str, tuple[...]]` to `dict[tuple[str, str], tuple[float, str | None, str | None]]`, keyed by `(server_name, package_name)`. This is the core of the fix: **a cross-package entry becomes unrepresentable rather than something each reader must remember to detect**. The three readers stop needing an identity check because the key carries it. +- `_stale_cache_key` — **add** — small helper deriving `(server_name, package)` from a server name plus resolved command/args via the existing `detect_package_type`. Single place that decides identity, so the readers cannot drift apart again (the failure mode of every prior round). +- `_run_stale_index` (`~1339`) — **modify** — resolve the effective target **before** the TTL check, then look up by the composite key. Reordering is load-bearing: it is what stops a warm poisoned entry surviving unexamined. +- `_get_update_warning` (`~3713`) — **modify** — same composite-key lookup. Delete the `_notice_package_matches_cache` / `_repoint_cache_entry_to_effective_package` pair added in #154; both become unnecessary, and the second was actively harmful (it erased a good version baseline). +- `catalog_search` `stale_updates` (`~1698`) — **modify** — this is the reader that leaked in all three rounds. It must apply the **same TTL** as the other two (entries older than `_stale_check_ttl_seconds` are not emitted) and, because the key now carries the package, it can no longer attribute an A-derived tuple to B. Extract the shared predicate so all three readers provably agree. +- `update_server` success path (`~5442-5452`) — **modify** — write the repopulated entry under the composite key. Its version write to `desc_entry.version` (`5394`) stays; the erasure problem disappears with `_repoint_...`. +- `_effective_notice_target` (added in #154, `~3640`) — **keep** — the `.mcp.json`-over-manifest precedence and the `is_server_allowed` gate were accepted in every round. Unchanged. +- `update_server` drift recheck (`~5323-5350`) — **keep** — the #151 fix as reworked: recheck after the probe *and* after the version lookup, compared via `_refresh_config_unchanged` (covers `cwd` and effective `env`, not just argv). Accepted by round 3. + +### `tests/test_tools.py` (modify) + +Every test must assert an **observable notice outcome**. Three rounds were passed by tests that asserted a queried argument or a mutated label while the bug survived; that shape is banned here. + +- **add** — `test_catalog_search_never_serves_a_cross_package_tuple`: pre-populate a warm A-derived tuple, switch config to B, assert `catalog_search().stale_updates` is empty. This is the exact case that survived rounds 1–3. +- **add** — `test_catalog_search_applies_the_same_ttl_as_other_readers`: an expired entry must not be emitted. +- **add** — `test_notice_resumes_after_package_change`: config switches A→B; assert that within the normal flow a **real notice for B is eventually emitted** (not merely that a label changed). This is the assertion whose absence let the permanent-silence bug pass. +- **add** — `test_update_server_version_survives_subsequent_warning`: after a successful override update writes B's version, a following `_get_update_warning` must not erase it. Pins the erasure codex reproduced. +- **add** — `test_local_to_remote_override_evicts_stale_tuple`: local A → remote override; assert no A notice from any reader. Round 3 found this leaking. +- **keep** — the accepted #151 tests (`aborts_when_config_changes_during_probe`, `aborts_on_env_only_drift`, `restart_consumes_the_confirmed_config`) and the accepted #150 precedence/policy/remote tests. + +## Documentation impact + +- `CHANGELOG.md` — **modify** — entries under `## [Unreleased]` for #150 and #151. **Mandatory**: `main` is protected and the `changelog` check fails any PR touching `src/` without it. The #150 entry must describe the actual behaviour — notices resume once a version for the new package is observed — and must **not** repeat the `pmcp refresh` recovery claim, which I verified is false for override-named packages. + +## Frozen vocabulary / protocol check + +`_stale_check_cache` is private in-memory state with no serialised form: it is not persisted, not in `types.py`, and not exposed in any tool schema. Re-keying it changes **no** protocol surface. `CatalogSearchOutput.stale_updates` keeps its existing type (`list[str] | None`) and message format. **No new vocabulary is introduced.** + +## Dependencies & order + +1. Add `_stale_cache_key`, then re-key the cache and migrate all three readers **in one commit** — a half-migrated cache mixes key shapes and would fail confusingly. +2. Reorder `_run_stale_index` (resolve before TTL) in the same commit; eviction is not what fixes it, ordering is. +3. Remove `_notice_package_matches_cache` and `_repoint_cache_entry_to_effective_package`. +4. Tests, each proven RED against the pre-fix source. +5. CHANGELOG last. + +No external dependencies. No migration: the cache is in-memory and rebuilt on start. + +## Verification + +```bash +cd + +uv run pytest tests/test_tools.py -q -k 'catalog_search or notice or stale or update_server' + +# RED proof — required; a passing test is not evidence the test works. Revert +# ONLY src/pmcp/tools/handlers.py via a targeted `git diff > patch` + `git apply -R`. +# NEVER `git checkout HEAD -- .` (it has destroyed uncommitted work in this repo). +# Each new test must fail against the pre-fix source; record the actual output. + +uv run pytest -q # baseline 2536 + new tests, 3 skipped, 0 failed +uv run ruff check . +uv run ruff format --check src/ tests/ +uv run mypy src +``` + +Beyond the suite, reproduce the round-3 defect by hand and confirm it is gone: drive `_get_update_warning` three consecutive times across an A→B config switch and assert a notice for B is eventually produced (the failing transcript printed `None` three times). + +Edge cases: server in manifest only; `.mcp.json` only; both with the same package (must not regress — the common case); remote override; policy-denied; absent everywhere; an entry whose package cannot be detected (`detect_package_type` → `None`). + +## Acceptance criteria + +- [ ] No reader can emit a notice comparing versions from two different packages — proven by `test_catalog_search_never_serves_a_cross_package_tuple` and `test_local_to_remote_override_evicts_stale_tuple`, both failing when the src change is reverted. +- [ ] After a package change, notices **resume** rather than stopping permanently — proven by `test_notice_resumes_after_package_change` asserting an emitted notice, not a mutated field. +- [ ] A version baseline written by a successful `update_server` is not erased by a later notice check — proven by `test_update_server_version_survives_subsequent_warning`. +- [ ] `catalog_search` honours the same TTL as the other two readers — proven by `test_catalog_search_applies_the_same_ttl_as_other_readers`. +- [ ] Full suite ≥2536 + new tests passed / 3 skipped / 0 failed, with `ruff check`, `ruff format --check src/ tests/`, `mypy src` clean, and a `## [Unreleased]` CHANGELOG entry present. + +## Execution Policy + +- execute: effort=high, reason=three prior board rounds each rejected a patch at this layer; the change re-keys shared cache state read by three call sites with differing contracts, and the failure mode is a silently wrong user-facing notice. + +## Automation + +```yaml +automation: + suite_command: "uv run pytest -q" +``` + +--- + +## Board review — REJECTED (3/3 DISAGREE). Do not implement as written. + +Boarded before implementation. grok-4.6, codex and gemini all blocked, and two converged independently on a flaw that is fatal to this plan's central claim. Verified against source; recorded here so the next attempt does not repeat it. + +### B1 (fatal) — re-keying the cache does NOT make a cross-package comparison unrepresentable + +The plan's whole premise. It is wrong. Both writers still source `current` from the single server-scoped `GeneratedServerDescriptions.version`: + +- `_run_stale_index` (`handlers.py:1367`) writes `server_desc.version` as `current` +- `_get_update_warning` (`handlers.py:3782`) reads `self._descriptions_cache.servers[server_name].version` + +So after an A→B switch this plan stores `(A_version, B_latest)` under key `(server, B)` — the composite key makes the entry look authoritative while the comparison remains exactly as wrong as before. Re-keying relabels the poison; it does not prevent it. + +### B2 (root cause, and the reason three fixes failed) — nothing observes an INSTALLED version + +grok's diagnosis, which is more precise than my framing: `get_package_version` returns **upstream latest only**. There is no code path anywhere that observes the *installed* version of a package. `GeneratedServerDescriptions.version` is itself an upstream snapshot from the last describe pass, not an installed version. + +That is why every attempt to "establish a baseline for B" has failed, including the #154 self-heal: **the baseline does not exist to be established.** `update_server` cannot serve as the recovery either — notices must work *before* an update, and its success path writes only `version`, never the description's `package` (`handlers.py:5393`). + +This reframes #150. It is not a cache-keying bug. The system compares an upstream-latest snapshot taken at describe time against an upstream-latest fetched now, and calls the difference an available update. That works only while the described package and the configured package are the same. Any real fix must decide what "currently installed version" means and where it comes from — a question this plan never asks. + +### B3 — `(server, package_name)` is not a complete identity anyway + +`detect_package_type` distinguishes ecosystems while allowing identical name strings across them. Verified: + +``` +npx foo -> ('npm', 'foo') +uvx foo -> ('pypi', 'foo') # same package_name, different ecosystem +``` + +Switching `npx foo` to `uvx foo` reuses `(server, "foo")` and compares versions from different registries. The key would need the package *type* too — but per B1/B2 the key is not where the fix lives. + +### Also raised + +- Orphaned old-package keys stay user-visible (`catalog_search` enumerates every raw entry) and grow unboundedly; TTL hides them after 6h but never removes them. +- The acceptance criteria as written are not falsifying — the resume criterion cannot be honestly satisfied while B2 stands. + +### Status + +Plan rejected, not implemented. #150 needs a decision about installed-version provenance before any further code. #151's fix is independent of all of this and was accepted in every round. diff --git a/.consiliency/plans/detailed-update-server-resolution-residuals-20260816-1824.md b/.consiliency/plans/detailed-update-server-resolution-residuals-20260816-1824.md new file mode 100644 index 00000000..ba4eb3ea --- /dev/null +++ b/.consiliency/plans/detailed-update-server-resolution-residuals-20260816-1824.md @@ -0,0 +1,119 @@ +# Detailed plan: unify update_server config resolution across notice paths and the probe window + +## Task + +Close pmcp issues #150 and #151, the two residuals filed during the PR #147 board review. + +**#150** — `gateway.update_server` resolves its target through `_resolve_lifecycle_config` (which gives `.mcp.json` precedence over the manifest), but the two *notice* paths still use the manifest-only `_get_server_config_for_update`: the background stale sweep (`handlers.py:1350`) and `_get_update_warning` (`handlers.py:3622`). Once the 6h stale-check TTL expires, an override naming a **different package** than the manifest entry compares unrelated packages — resurfacing a false "update available" notice or hiding a real one. + +**#151** — `update_server` resolves config once for the probe (around `handlers.py:5059`) and then `restart_server` independently re-resolves (`handlers.py:2914`) after a probe with a 60s timeout. A config edit landing inside that window can probe package A and restart package B, then persist A's version for B — the same silent-misreport class #147 existed to eliminate. + +## Research summary + +Reconnaissance was done inline (files already in session from the #147 work) rather than via Explore teammates, per the skill's "skip when context is already in session" rule. + +The load-bearing finding is that **`_resolve_lifecycle_config` is not a drop-in replacement for the notice paths.** Read at `handlers.py:3577+`, it returns `tuple[ResolvedServerConfig | None, LifecycleServerOutput | None]` and produces *failure outputs* for three conditions: policy denial (`is_server_allowed`), missing remote-header env vars (`_missing_remote_header_env_vars`), and a configured-duplicate credential gap (`_configured_duplicate_missing_credential`). Those are correct for a lifecycle **action**, but wrong for a best-effort **notice** — a policy-denied or credential-gapped server should simply produce no notice, not a suppressed one for an unrelated reason, and the notice paths have no channel for a `LifecycleServerOutput`. + +The precedence itself comes from `_load_all_configured_servers` (`handlers.py`, immediately after `_resolve_lifecycle_config`), which is a thin wrapper over `load_configs(project_root=..., custom_config_path=...)` plus `filter_self_references`. That is the piece the notice paths actually need. + +Both notice call sites consume only `.command` and `.args` (verified at `handlers.py:1350-1356` and `handlers.py:3622-3625`). `ResolvedServerConfig` (`types.py:319`) wraps a `McpServerConfig` union; the local arm `LocalMcpServerConfig` (`types.py:178`) carries `command`/`args`, while a remote arm carries neither — so a remote override must resolve to "no notice", matching today's behavior when `server_config.command` is falsy. + +For #151, `restart_server` (`handlers.py:2908`) resolves and then calls `self._client_manager.restart_server(config, force=...)`. The `ClientManager` method takes an already-resolved config, so threading the caller's config through avoids the second resolution without duplicating the handler's policy/credential gating. + +## Board review — three blocking defects found, plan revised + +This plan was reviewed by the advisor panel (grok-4.6 AGREE, codex DISAGREE, gemini DISAGREE) before implementation. Two legs independently found the same policy-gating contradiction; codex found two further defects. **All three were verified against source and the plan below is the revised version.** Recorded here because the original design would have shipped an incomplete fix. + +**B1 — the notice fix was half a fix (codex).** Changing only the *latest* lookup to the configured package leaves *current* as `GeneratedServerDescriptions.version`, which was generated from the **manifest** package (`refresher.py`). So manifest package A at `10.0.0` vs configured package B at `2.0.0` still compares unrelated packages and can hide B's real update. `GeneratedServerDescriptions` carries a `package` field (`types.py:1365`) that records which package the entry describes — the original plan never consulted it. A test asserting "B was queried" would have passed while the bug survived. + +**B2 — the #151 fix could restart the wrong package (codex).** `update_server` calls `self.refresh(...)` *after* the probe (`handlers.py:5167`), which reloads config and applies an edit to B. Passing the stale probed A into the restart would then disconnect B and reconnect A — worse than the TOCTOU it fixes — and would bypass restart-time credential gating. + +**B3 — the policy-silence criterion was unsatisfiable (codex + gemini).** `_load_all_configured_servers` returns entries *before* policy filtering (its own docstring says so). A helper doing no gating would return command/args for a denied server, so `test_notice_paths_stay_silent_for_policy_denied_server` could not pass as specified, and denied servers would leak notices. + +## Changes + +### `src/pmcp/tools/handlers.py` (modify) + +- `_effective_notice_target` — **add** — new private helper returning `tuple[str, list[str]] | None` (command, args). Applies `.mcp.json`-over-manifest precedence via `_load_all_configured_servers`, falling back to manifest → discovered as `_get_server_config_for_update` does today. Returns `None` when: nothing resolves; the effective config is remote (no `command`); the command is empty; **or the server is not `self._policy_manager.is_server_allowed(...)`** — a pure predicate (`policy.py:104`), so this is the side-effect-free eligibility check B3 requires, without importing `_resolve_lifecycle_config`'s failure-output machinery. Credential/header gaps are deliberately *not* checked: they do not change which package is installed, and a notice for a credential-gapped server is still truthful. +- `_notice_package_matches_cache` — **add** — returns whether the effective config's detected package equals the `package` recorded on the descriptions-cache entry. Fixes **B1**: `_get_update_warning` and `_run_stale_index` must compare like with like. Uses the existing `detect_package_type` to derive the effective package name. +- `_run_stale_index` (call site around `handlers.py:1350`) — **modify** — resolve through `_effective_notice_target`; **skip the server entirely when the package identity does not match** the cache entry, rather than writing a cross-package `(current, latest)` pair. Keeps the existing `continue`-on-`None`. +- `_get_update_warning` (call site around `handlers.py:3622`) — **modify** — same resolution and the same identity guard; return `None` on mismatch (fail silent — a wrong notice is worse than none). Keeps the existing `return None`-on-`None`. +- `_get_server_config_for_update` — **keep, unmodified** — still used by `update_server`'s manifest-oriented lookups; docstring gains one line pointing notice paths at the new helper. +- `update_server` (around `handlers.py:5059`) — **modify** — fixes **B2**. After the probe and `refresh()`, **re-resolve** the config and compare it against the probed snapshot (command + args). If it changed, abort with `ok=False` and a message naming the drift, and mutate **no** cache or stale-check state. Only when unchanged, pass the (now confirmed-current) config to the restart. This closes #151's window by *detecting* the edit rather than by pinning a stale config through it. +- `restart_server` (`handlers.py:2908`) — **modify** — add a keyword-only `resolved_config: ResolvedServerConfig | None = None`, used in place of the internal `_resolve_lifecycle_config` call when supplied. `update_server` passes the re-resolved-and-confirmed config. When `None`, behavior is byte-identical to today. Note per gemini: this is safe *because* `update_server` performs the same gating first; the parameter must stay internal and never reach the public tool schema. + +### `tests/test_tools.py` (modify) + +Tests assert **observable outcomes** (was a notice emitted? what did the cache record?), never merely "package B was queried" — per the board, an argument-plumbing assertion passes while the underlying bug survives. + +- `TestUpdateServerVersionRepair` (or a sibling class) — **add** — `test_notice_paths_use_configured_override_package`: manifest names A, `.mcp.json` names B, cache entry's `package` is B; assert `_get_update_warning` reports B's real update. +- **add** — `test_notice_suppressed_when_cache_package_mismatches_config` (**B1**): cache entry describes manifest package A at `10.0.0`; effective config is package B at `2.0.0`. Assert **no notice** is emitted and no `_stale_check_cache` entry pairs A's version with B's latest. Without the identity guard this produces a wrong notice — the exact hidden/false-notice failure #150 describes. +- **add** — `test_stale_sweep_skips_package_identity_mismatch`: same fixture driving `_run_stale_index`; assert the server is skipped rather than cached with a cross-package pair. +- **add** — `test_notice_paths_stay_silent_for_policy_denied_server` (**B3**): a policy-denied server present in `.mcp.json` yields no notice and raises nothing. Pins the `is_server_allowed` check; without it the helper returns command/args and the notice leaks. +- **add** — `test_notice_paths_ignore_remote_override`: a remote (URL-based) override yields no notice rather than an `AttributeError`. +- **add** — `test_update_server_aborts_when_config_changes_during_probe` (**B2**): resolve A, then mutate the config to B before the restart. Assert `ok is False`, the message names the drift, and **no** cache/stale-check mutation occurred. Without the re-resolve-and-compare, the old code restarts A over B's config. +- **add** — `test_update_server_restart_consumes_the_confirmed_config`: unchanged-config happy path — assert `restart_server` receives the caller's config and resolution is not repeated. + +## Documentation impact + +- `CHANGELOG.md` — **modify** — add entries under `## [Unreleased]` for both fixes. **Mandatory**: `main` is protected and the required `changelog` check fails any PR touching `src/` without a `CHANGELOG.md` change. + +No other cross-cutting doc applies: `README.md` documents `gateway.update_server`'s user-facing contract, which is unchanged (no new parameter, no changed output shape — `restart_server`'s new argument is internal, keyword-only, and defaulted). + +## Frozen vocabulary / protocol check + +`update_server`'s public tool schema (`get_gateway_tool_definitions`, around `handlers.py:613`) and `UpdateServerOutput`/`RestartServerInput` in `types.py` are the protocol surface. **No new vocabulary is introduced**: the `resolved_config` parameter is a Python keyword-only argument on an internal method, never a JSON-RPC field, and no output field is added, removed, or renamed. + +## Dependencies & order + +1. Add `_effective_notice_target` (with the `is_server_allowed` gate) and `_notice_package_matches_cache` first — both #150 call-site changes depend on them. +2. Migrate the two notice call sites, including the identity guard. +3. Then #151: re-resolve-and-compare in `update_server`, plus the optional `resolved_config` parameter on `restart_server`. Independent of #150; ordered second so a RED check for #150 is not entangled with a signature change. +4. CHANGELOG last, so the entry describes what actually landed. + +No external/blocking dependencies — no migrations, no config changes, no upstream coordination. + +## Verification + +```bash +cd /home/viperjuice/code/pmcp + +# Targeted first (fast loop) +uv run pytest tests/test_tools.py -q -k 'notice or stale_sweep or update_server or restart' + +# RED proof — required by this repo's standard; a passing test is not evidence +# the test works. Revert ONLY the src change, keep tests, observe failures, +# restore. Do NOT use `git checkout HEAD -- .` (it has destroyed uncommitted +# work in this repo before); revert with a targeted `git diff > patch` + +# `git apply -R` on src/pmcp/tools/handlers.py only. +uv run pytest tests/test_tools.py -q -k 'notice or stale_sweep' # expect RED without the #150 fix +uv run pytest tests/test_tools.py -q -k 'restart_uses_the_probed' # expect RED without the #151 fix + +# Full gates +uv run pytest -q # expect 2536 + new tests passed, 3 skipped, 0 failed +uv run ruff check . +uv run ruff format --check src/ tests/ +uv run mypy src +``` + +Edge cases to exercise: server present in manifest only (unchanged behavior); present in `.mcp.json` only; present in both with the *same* package (must stay silent — this is the common case and must not regress); remote override; policy-denied server; server absent everywhere. + +## Acceptance criteria + +- [ ] `_get_update_warning` and `_run_stale_index` resolve a server's package through the `.mcp.json`-over-manifest precedence — proven by `uv run pytest tests/test_tools.py -q -k 'notice or stale_sweep'` passing, and failing when the src change alone is reverted. +- [ ] A notice is never emitted from a cross-package comparison: when the descriptions-cache entry's `package` differs from the effective config's package, both notice paths stay silent — proven by `test_notice_suppressed_when_cache_package_mismatches_config` and `test_stale_sweep_skips_package_identity_mismatch`. +- [ ] A policy-denied or remote-override server yields no notice and raises nothing — proven by `test_notice_paths_stay_silent_for_policy_denied_server` and `test_notice_paths_ignore_remote_override`. +- [ ] A config edit landing between the probe and the restart aborts the update with `ok=False` and mutates no cache state — proven by `test_update_server_aborts_when_config_changes_during_probe`, failing when the #151 change alone is reverted. +- [ ] Full suite is 2536 + new tests passed / 3 skipped / 0 failed, with `ruff check`, `ruff format --check src/ tests/`, and `mypy src` all clean. +- [ ] `CHANGELOG.md` has entries under `## [Unreleased]` for both issues, so the required `changelog` check passes on the PR. + +## Execution Policy + +- execute: effort=medium, reason=two small resolution changes in a subsystem with a documented history of silent-misreport regressions; mechanical edits but the failure mode is subtle and the no-gating design decision must be preserved. + +## Automation + +```yaml +automation: + suite_command: "uv run pytest -q" +``` diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 479fbfba..b99b04ea 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -220,7 +220,14 @@ jobs: # same frozen payload rather than re-fetching it, so a label applied to an # already-running/failed PR would never be seen that way. Reading live here # means "apply the label, then re-run the job" actually works. - LABELS=$(gh api "repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels" --jq '.[].name') + # Use the PULLS endpoint, not /issues//labels: this job grants + # `pull-requests: read`, and the issues endpoint needs `issues: read`, + # so it returned "Validation Failed (HTTP 422)". Under `bash -e` that + # non-zero exit killed the step BEFORE the CHANGELOG check ran, so the + # guard failed on its own permissions rather than on the PR's content. + # `|| true` keeps a future API hiccup from doing the same: a lookup + # failure must not decide the check, it just means "no label seen". + LABELS=$(gh api "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}" --jq '.labels[].name' || true) echo "Current labels: $LABELS" if echo "$LABELS" | grep -qx 'skip-changelog'; then echo "skip-changelog label present — skipping the CHANGELOG.md check." diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f1e338c..90938745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **Update notices are no longer fabricated for servers whose version is not a + release number.** The version comparison extracted digits from whatever it was + given and had no way to say "I cannot read this", so a server reporting + `nightly`, `build-1`, `main`, or an empty string compared as *older* than any + real release and produced an "update available" notice that was never true. + An empty version is the default for servers built on the current MCP SDK, so + this was reachable in ordinary use. + + Comparison now uses `packaging.version` (a new dependency), which implements + PEP 440 ordering and rejects anything that is not a release outright. Beyond + removing the false notices, this corrects ordering that the previous + digit-extraction approach got wrong: a pre-release such as `1.0.0-rc1` is now + correctly older than `1.0.0` (previously the real release was hidden), and a + build-metadata difference is no longer announced as a new release. Docker + images, whose "version" is a content digest rather than a number, are compared + for difference instead of order. + ## [2.1.1] - 2026-08-12 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 81278e67..a4b9e278 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,12 @@ classifiers = [ "Typing :: Typed", ] dependencies = [ + # Version ordering for update notices. Hand-rolled digit extraction + # fabricated "update available" notices for any non-numeric version + # (`nightly`, `build-1`, and the empty string the MCP SDK defaults to); + # packaging implements PEP 440 ordering and rejects non-versions outright. + # Pure-Python, no transitive deps, already ubiquitous in the ecosystem. + "packaging>=22", # Upper bound is load-bearing: 3.0.0 is unreleased and unaudited. pmcp's # P2 phase ported the gateway's upstream handlers and downstream # transport onto mcp 2.0.0's rewritten low-level Server (typed on_* diff --git a/src/pmcp/manifest/refresher.py b/src/pmcp/manifest/refresher.py index 2009b708..b1373b5f 100644 --- a/src/pmcp/manifest/refresher.py +++ b/src/pmcp/manifest/refresher.py @@ -21,6 +21,7 @@ detect_package_type, get_package_version, is_version_newer, + is_version_orderable, ) from pmcp.types import ( DescriptionsCache, @@ -216,7 +217,17 @@ async def refresh_server( server_config.command, server_config.args ) - if version and not is_version_newer(existing_cache.version, version): + # `not is_version_newer(...)` means "not newer", which is NOT the same as + # "up to date" now that the comparator fails closed: an unorderable + # cached version (e.g. the literal "unknown" this function persists + # below after a failed lookup) yields False and would pin the stale + # cache forever. Require the cached version to be orderable before + # trusting "not newer" as up-to-date (board review round 4). + if ( + version + and is_version_orderable(existing_cache.version) + and not is_version_newer(existing_cache.version, version, pkg_type) + ): logger.debug( f"Server {server_name} is up to date (v{existing_cache.version})" ) @@ -411,11 +422,11 @@ async def check_staleness( if not server_config: continue - version, _ = await get_package_version( + version, pkg_type = await get_package_version( server_config.command, server_config.args ) - if version and is_version_newer(desc.version, version): + if version and is_version_newer(desc.version, version, pkg_type): stale_servers[name] = (desc.version, version) logger.info(f"Server {name} is stale: {desc.version} -> {version}") diff --git a/src/pmcp/manifest/version_checker.py b/src/pmcp/manifest/version_checker.py index 062d3107..68b61f4c 100644 --- a/src/pmcp/manifest/version_checker.py +++ b/src/pmcp/manifest/version_checker.py @@ -9,6 +9,7 @@ from urllib.parse import quote import aiohttp +from packaging.version import InvalidVersion, Version from pmcp import __version__ @@ -415,39 +416,191 @@ async def get_package_version( return (None, "unknown") -def is_version_newer(current: str, latest: str) -> bool: +# `get_docker_version` returns a BARE hex digest -- it strips the `sha256:` +# prefix and truncates to 12 chars (see its implementation, pinned by +# TestGetDockerVersion). Matching on a `sha256:` prefix therefore never fires +# against the real producer; an earlier version of this guard did exactly that +# and silenced every docker update notice. Accept the bare form, and the +# prefixed form for robustness if the producer ever stops truncating. +# Shape alone cannot separate a digest from a version: `get_docker_version` +# truncates SHA-256 to 12 hex chars, which can be ALL NUMERIC (`987654321098`), +# while `202612180000` is a plausible calendar version. An earlier attempt +# required a hex letter and thereby dropped real all-numeric digest changes. +# So shape is only a candidate test -- `package_type` decides, and callers have +# it (board review round 4). +_DIGEST_RE = re.compile(r"^(?:sha256:)?[0-9a-f]{12,64}$") +_DIGEST_LETTER_RE = re.compile(r"^(?:sha256:)?(?=[0-9a-f]*[a-f])[0-9a-f]{12,64}$") + + +def _digest_identity(value: str, package_type: str | None = None) -> str | None: + """Canonical digest identity for *value*, or ``None`` if it is not a digest. + + Canonicalises so the SAME image never reads as an update just because the + producer's representation changed: the `sha256:` prefix is dropped and the + hex truncated to the 12 chars `get_docker_version` emits. Without this, + `abcdef123456` vs `sha256:abcdef123456` -- one image, two spellings -- + compares as a new release. + + A digest is an identity, not an ordinal: "newer" can only mean "different". """ - Compare versions to check if latest is newer than current. + candidate = (value or "").strip() + # With a known docker package type, any hex-shaped value is a digest -- + # including an all-numeric truncation. Without one, require a hex letter so + # a numeric calendar version is not misread as a digest and silently + # excluded from ordering. + pattern = _DIGEST_RE if package_type == "docker" else _DIGEST_LETTER_RE + if not pattern.match(candidate): + return None + hex_part = candidate[7:] if candidate.startswith("sha256:") else candidate + return hex_part[:12] - Handles common version formats: - - Semver: 1.2.3, 0.0.19 - - Date-based: 2025.12.18 - Args: - current: Current cached version - latest: Latest available version +def _parse_version(value: str) -> Version | None: + """Parse *value* as a release version, or ``None`` if it is not one. - Returns: - True if latest > current + Uses ``packaging.version``, which implements PEP 440 ordering (including + pre-release, post-release and dev precedence) and raises on anything that + is not a version. Hand-rolled digit extraction was tried three times and + produced a fabricated-notice bug every time: it silently turned `build-1` + into `(1,)`, `2026-08-17-nightly` into `(2026, 8, 17)`, and `""` into `()`, + each of which compared as OLDER than a real release. + + A leading ``v`` is accepted (``v1.2.3``) since registries commonly emit it. + """ + candidate = (value or "").strip() + if not candidate: + return None + candidate = re.sub(r"^[vV]", "", candidate) + try: + return Version(candidate) + except InvalidVersion: + return None + + +# npm and Cargo publish SemVer, where `-anything` is a PRERELEASE ordered BELOW +# the release. PEP 440 (what `packaging` implements) reads `1.0.0-1` as the +# POST-release `1.0.0.post1` -- the opposite order. 79 of the manifest's 107 +# servers are npm, so using PEP 440 for them inverts precedence on a real +# format: it hides `1.0.0-1 -> 1.0.0` and fabricates the reverse. +_SEMVER_ECOSYSTEMS = frozenset({"npm", "cargo"}) + +# SemVer 2.0.0 rules 2, 9 and 10: core identifiers are non-negative integers +# WITHOUT leading zeros; numeric prerelease identifiers likewise; and identifiers +# must not be empty. An earlier version accepted `1.0.0-01`, `01.0.0` and +# `1.0.0-a..b` and ORDERED them, fabricating updates from strings npm could +# never have published (board review round 4). +_SEMVER_CORE = r"(0|[1-9]\d*)" +_SEMVER_PRE_IDENT = r"(?:0|[1-9]\d*|\d*[A-Za-z-][0-9A-Za-z-]*)" +_SEMVER_RE = re.compile( + rf"^[vV]?{_SEMVER_CORE}\.{_SEMVER_CORE}\.{_SEMVER_CORE}" + rf"(?:-({_SEMVER_PRE_IDENT}(?:\.{_SEMVER_PRE_IDENT})*))?" + r"(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$" +) + + +def _is_semver_ecosystem(package_type: str | None) -> bool: + return package_type in _SEMVER_ECOSYSTEMS + + +def _semver_key(value: str) -> tuple[tuple[int, int, int], list[object]] | None: + """SemVer 2.0.0 precedence key, or ``None`` if *value* is not SemVer. + + Build metadata is ignored (spec: it does not affect precedence). A release + outranks any prerelease of the same core version; prerelease identifiers + compare numerically when numeric, else lexically, with numeric ranked lower. + """ + match = _SEMVER_RE.match((value or "").strip()) + if not match: + return None + core = (int(match.group(1)), int(match.group(2)), int(match.group(3))) + prerelease = match.group(4) + if prerelease is None: + # No prerelease sorts ABOVE any prerelease of the same core. + return (core, []) + parts: list[object] = [] + for ident in prerelease.split("."): + parts.append((0, int(ident), "") if ident.isdigit() else (1, 0, ident)) + return (core, parts) + + +def _semver_is_newer(current: str, latest: str) -> bool: + """SemVer precedence, failing closed when either side is not SemVer.""" + current_key = _semver_key(current) + latest_key = _semver_key(latest) + if current_key is None or latest_key is None: + return False + current_core, current_pre = current_key + latest_core, latest_pre = latest_key + if latest_core != current_core: + return latest_core > current_core + # Same core: a release (empty prerelease list) outranks any prerelease. + if not latest_pre: + return bool(current_pre) + if not current_pre: + return False + return latest_pre > current_pre # type: ignore[operator] + + +def is_version_orderable(value: str, package_type: str | None = None) -> bool: + """Whether *value* can be ordered at all -- a release version or a digest. + + Exists because ``not is_version_newer(a, b)`` is ambiguous under a + fail-closed comparator: it is True both when *a* is genuinely current AND + when *a* is unreadable. A caller that treats "not newer" as "up to date" + must first check that the value is orderable, or an unreadable version + (`"unknown"`, `""`) reads as current forever. + """ + if _digest_identity(value, package_type) is not None: + return True + if _is_semver_ecosystem(package_type): + return _semver_key(value) is not None + return _parse_version(value) is not None + + +def is_version_newer( + current: str, latest: str, package_type: str | None = None +) -> bool: + """Whether *latest* is a strictly newer release than *current*. + + FAILS CLOSED. Returning ``True`` tells an operator their server is out of + date, so anything this function cannot actually order returns ``False`` + (Consiliency/pmcp#150 board review). An unreadable version produces no + notice rather than a false one. + + * Both sides parse as versions -> PEP 440 ordering, so `1.0.0-rc1` is + correctly OLDER than `1.0.0`, and build metadata is not precedence. + * Both sides are digests -> any difference is a new image. This is the + docker lane: `get_docker_version` returns a bare 12-hex digest. + * Anything else -- unparseable, empty (the mcp 2.x default + ``serverInfo.version``), or a digest compared against a version -> no + update. """ if current == latest: return False - # Try to parse as semver-like (X.Y.Z or X.Y) - def parse_version(v: str) -> tuple[int, ...]: - # Remove any non-numeric prefixes (v1.0.0 -> 1.0.0) - v = re.sub(r"^[vV]", "", v) - # Extract numeric parts - parts = re.findall(r"\d+", v) - return tuple(int(p) for p in parts) + current_digest = _digest_identity(current, package_type) + latest_digest = _digest_identity(latest, package_type) + if current_digest is not None or latest_digest is not None: + # Comparable only when BOTH are digests; then any difference in the + # CANONICAL identity is a new image. A digest and a version describe + # different things and are not ordered. + if current_digest is None or latest_digest is None: + return False + return current_digest != latest_digest + + if _is_semver_ecosystem(package_type): + return _semver_is_newer(current, latest) + + current_version = _parse_version(current) + latest_version = _parse_version(latest) + if current_version is None or latest_version is None: + return False - try: - current_parts = parse_version(current) - latest_parts = parse_version(latest) - return latest_parts > current_parts - except (ValueError, TypeError): - # Fall back to string comparison - return latest > current + # Compare on the PUBLIC segment: a local/build-metadata difference + # (`1.0.0+build.4` -> `+build.5`) is not a new release, and announcing one + # would be the same fabricated notice this function exists to prevent. + # Pre-release precedence is retained, since `.public` keeps it. + return Version(latest_version.public) > Version(current_version.public) def clear_version_cache() -> None: diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index 8858cba4..08367ec2 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -1085,7 +1085,14 @@ def __init__( # so concurrent polls cannot double-adopt or re-refresh a finished job. self._provision_finalize_locks: dict[str, asyncio.Lock] = {} self._provision_finalized: set[str] = set() - self._stale_check_cache: dict[str, tuple[float, str | None, str | None]] = {} + # (checked_at, current, latest, package_type). The package type is + # carried so the READ sites can order correctly: npm/cargo publish + # SemVer, where `-1` is a prerelease, while PEP 440 reads it as a + # post-release. Without it, catalog_search inverts npm prereleases -- + # hiding a real upgrade and fabricating its reverse (ah board review). + self._stale_check_cache: dict[ + str, tuple[float, str | None, str | None, str | None] + ] = {} self._stale_check_ttl_seconds = 6 * 60 * 60 self._stale_index_interval_seconds = 60 * 60 # Re-index every hour self._stale_index_task: asyncio.Task[None] | None = None @@ -1351,15 +1358,16 @@ async def _run_stale_index(self) -> None: if not server_config or not server_config.command: continue try: - latest, _ = await get_package_version( + latest, pkg_type = await get_package_version( server_config.command, server_config.args, timeout=5.0 ) self._stale_check_cache[server_name] = ( now, server_desc.version, latest, + pkg_type, ) - if latest and is_version_newer(server_desc.version, latest): + if latest and is_version_newer(server_desc.version, latest, pkg_type): logger.info( f"Update available for '{server_name}': " f"{server_desc.version} -> {latest}" @@ -1689,10 +1697,12 @@ def sort_key(t: Any) -> tuple[int, int, str, str]: stale = [ f"Update available for '{sn}': {current} -> {latest}. " f"Call gateway.update_server(server_name='{sn}') to update." - for sn, (_, current, latest) in self._stale_check_cache.items() + for sn, (_, current, latest, pkg_type) in ( + self._stale_check_cache.items() + ) if current and latest - and is_version_newer(current, latest) + and is_version_newer(current, latest, pkg_type) and self._policy_manager.is_server_allowed(sn) ] if stale: @@ -3634,19 +3644,25 @@ async def _get_update_warning(self, server_name: str) -> str | None: cached = self._stale_check_cache.get(server_name) if cached and (now - cached[0]) < self._stale_check_ttl_seconds: latest_cached = cached[2] - if latest_cached and is_version_newer(current_version, latest_cached): + cached_pkg_type = cached[3] + if latest_cached and is_version_newer( + current_version, latest_cached, cached_pkg_type + ): return ( f"Update available for '{server_name}': {current_version} -> {latest_cached}. " f"Call gateway.update_server with server_name='{server_name}'." ) return None - latest, _pkg_type = await get_package_version( + latest, pkg_type = await get_package_version( server_config.command, server_config.args, timeout=3.0 ) - self._stale_check_cache[server_name] = (now, current_version, latest) + self._stale_check_cache[server_name] = (now, current_version, latest, pkg_type) - if latest and is_version_newer(current_version, latest): + # Pass the ecosystem: npm/cargo publish SemVer, where `-1` is a + # PRERELEASE, while PEP 440 reads it as a POST-release. Without this the + # ordering inverts for the 79 npm servers in the manifest. + if latest and is_version_newer(current_version, latest, pkg_type): return ( f"Update available for '{server_name}': {current_version} -> {latest}. " f"Call gateway.update_server with server_name='{server_name}'." @@ -5253,6 +5269,7 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: now, latest_version, latest_version, + package_type, ) else: self._stale_check_cache.pop(server_name, None) diff --git a/tests/test_tools.py b/tests/test_tools.py index 2a905de9..0b5b33a8 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -4689,7 +4689,7 @@ async def fake_get_package_version(command, args, timeout=5.0): await gt._run_stale_index() assert "playwright" in gt._stale_check_cache - _, current, latest = gt._stale_check_cache["playwright"] + _, current, latest, _pkg = gt._stale_check_cache["playwright"] assert current == "0.1.0" assert latest == "0.2.0" @@ -4716,7 +4716,7 @@ async def test_run_stale_index_skips_fresh_cache(self, monkeypatch): self._make_manifest_with_playwright(monkeypatch) # Pre-populate cache as fresh - gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.1.0") + gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.1.0", "npm") network_called = [] @@ -4764,7 +4764,7 @@ async def test_catalog_search_includes_stale_updates(self, monkeypatch): ) # Pre-populate stale check cache with a stale entry - gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.2.0") + gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.2.0", "npm") result = await gt.catalog_search({}) @@ -4773,6 +4773,35 @@ async def test_catalog_search_includes_stale_updates(self, monkeypatch): assert "playwright" in result.stale_updates[0] assert "0.1.0" in result.stale_updates[0] assert "0.2.0" in result.stale_updates[0] + + @pytest.mark.asyncio + async def test_catalog_search_orders_npm_prereleases_by_semver(self) -> None: + """catalog_search must use the memo's ecosystem, not PEP 440. + + Board review: the memo previously stored only (checked_at, current, + latest), so this read site fell back to PEP 440. npm publishes SemVer, + where `1.0.0-1` is a PRERELEASE below `1.0.0`; PEP 440 reads it as the + POST-release `1.0.0.post1`. That inversion both HID the real + `1.0.0-1 -> 1.0.0` upgrade and FABRICATED its reverse -- reachable + through catalog_search even after the direct paths were fixed. + """ + client_manager = MockClientManager() + client_manager.set_server_online("playwright") + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + ) + + # A real npm upgrade: prerelease -> release. Must be reported. + gt._stale_check_cache["playwright"] = (time.time(), "1.0.0-1", "1.0.0", "npm") + result = await gt.catalog_search({}) + assert result.stale_updates is not None + assert "1.0.0-1" in result.stale_updates[0] + + # The reverse is NOT an upgrade and must not be announced. + gt._stale_check_cache["playwright"] = (time.time(), "1.0.0", "1.0.0-1", "npm") + result = await gt.catalog_search({}) + assert not (result.stale_updates or []) assert result.cli_hints == [] @pytest.mark.asyncio @@ -4801,7 +4830,7 @@ async def test_catalog_search_stale_updates_coexist_with_cli_hints( policy_manager=PolicyManager(), ) gt._detected_cli_infos = {"git": CLIInfo(name="git", path="/usr/bin/git")} - gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.2.0") + gt._stale_check_cache["playwright"] = (time.time(), "0.1.0", "0.2.0", "npm") monkeypatch.setattr( "pmcp.tools.handlers.load_manifest", create_manifest_for_request_tests ) @@ -4818,7 +4847,7 @@ async def test_catalog_search_no_stale_updates_when_up_to_date(self): import time gt = self._make_gateway_tools() - gt._stale_check_cache["playwright"] = (time.time(), "0.2.0", "0.2.0") + gt._stale_check_cache["playwright"] = (time.time(), "0.2.0", "0.2.0", "npm") result = await gt.catalog_search({}) diff --git a/tests/test_version_checker.py b/tests/test_version_checker.py index 86b14179..52ecd858 100644 --- a/tests/test_version_checker.py +++ b/tests/test_version_checker.py @@ -18,6 +18,7 @@ get_package_version, get_pypi_version, is_version_newer, + is_version_orderable, ) from pmcp import __version__ @@ -227,6 +228,194 @@ def test_mixed_versions(self) -> None: assert is_version_newer("1.0.0-rc1", "1.0.0-rc2") is True assert is_version_newer("v2.0-beta1", "v2.0-beta2") is True + # --- fail-closed: never fabricate an "update available" ------------------ + + def test_unorderable_current_reports_no_update(self) -> None: + """A version this function cannot order must NOT read as out of date. + + Consiliency/pmcp#150 board review. Returning True here makes the gateway + tell an operator their server is stale, so an unreadable version has to + report no update. Previously every one of these extracted digits (or an + empty tuple) and compared as OLDER than any real release, fabricating a + notice for any server whose version string is not a release number. + """ + for unreadable in ( + "nightly", + "release-channel-a", + "build-1", # contains a digit but is not a version + "main", + "latest", + "abc.def", + "2026-08-17-nightly", + ): + assert is_version_newer(unreadable, "2.0.0") is False, unreadable + + def test_empty_current_reports_no_update(self) -> None: + """The empty string is mcp 2.x's DEFAULT serverInfo.version. + + It is reached in practice by any server that does not set a version + explicitly, so it must not compare as older than every release. + """ + assert is_version_newer("", "2.0.0") is False + + def test_unorderable_latest_reports_no_update(self) -> None: + """An unreadable *latest* is equally unusable -- refuse to guess.""" + assert is_version_newer("1.0.0", "nightly") is False + assert is_version_newer("1.0.0", "") is False + + def test_docker_digests_compare_by_inequality(self) -> None: + """Digests are identities, not ordinals. + + Uses the BARE 12-hex form ``get_docker_version`` actually returns -- it + strips the ``sha256:`` prefix and truncates (see ``TestGetDockerVersion``). + A previous version of this guard matched only ``sha256:``-prefixed values + and was tested with invented literals, so it never fired against the real + producer and silenced every docker update notice. Ordering hex is + meaningless, but a DIFFERENT digest is genuinely a new image. + """ + assert is_version_newer("abcdef123456", "abcdef123456") is False + assert is_version_newer("abcdef123456", "fedcba654321") is True + # The prefixed/full form is still accepted, in case the producer ever + # stops truncating. + assert is_version_newer("sha256:abcdef123456", "sha256:fedcba654321") is True + + def test_digest_and_version_are_not_comparable(self) -> None: + """A digest and a release number describe different things.""" + assert is_version_newer("1.0.0", "abcdef123456") is False + assert is_version_newer("abcdef123456", "1.0.0") is False + + def test_docker_producer_output_is_orderable(self) -> None: + """Drive the PRODUCER, not a literal, so the two cannot drift apart. + + The prior bug was exactly this drift: the comparator matched a format + the producer does not emit. + """ + import re as _re + + from pmcp.manifest.version_checker import _digest_identity + + # Mirrors get_docker_version's truncation of a registry digest. + produced = "sha256:abcdef1234567890"[7:19] + assert _re.fullmatch(r"[0-9a-f]{12}", produced) + assert _digest_identity(produced) == produced + assert is_version_newer(produced, "0123456789ab") is True + + def test_long_numeric_version_is_not_mistaken_for_a_digest(self) -> None: + """A purely numeric string is a version, not a digest. + + `202612180000` is a plausible calendar/build stamp and is 12 chars of + `[0-9a-f]`. Without a hex-letter requirement it matches the digest + pattern, and is then never compared against a dotted release -- silently + dropping a real update. Found while reviewing my own digest rule. + """ + from pmcp.manifest.version_checker import _digest_identity + + assert _digest_identity("202612180000") is None + assert is_version_newer("202612180000", "202612190000") is True + + def test_semver_ecosystem_prerelease_ordering(self) -> None: + """npm/Cargo publish SemVer, where `-1` is a PRERELEASE. + + PEP 440 -- what `packaging` implements -- reads `1.0.0-1` as the POST + release `1.0.0.post1`, the opposite order. 79 of the manifest's 107 + servers are npm, so without an ecosystem-aware path this inverts + precedence on a real published format: it hides the `1.0.0-1 -> 1.0.0` + upgrade and fabricates the reverse. + """ + assert is_version_newer("1.0.0-1", "1.0.0", "npm") is True + assert is_version_newer("1.0.0", "1.0.0-1", "npm") is False + assert is_version_newer("1.0.0-alpha", "1.0.0-beta", "npm") is True + assert is_version_newer("1.0.0+build.4", "1.0.0+build.5", "npm") is False + # cargo shares SemVer semantics. + assert is_version_newer("1.0.0-1", "1.0.0", "cargo") is True + + def test_unorderable_version_is_not_reported_as_up_to_date(self) -> None: + """`not is_version_newer(...)` is ambiguous under a fail-closed comparator. + + Regression introduced by this PR and caught in review: `refresher` negates + the comparator, and once unreadable input returns False, `not(...)` reads + as "up to date". Since that function persists the literal `"unknown"` + after a failed lookup, the stale cache would be returned forever. + `is_version_orderable` is the discriminator callers need. + """ + assert is_version_orderable("unknown") is False + assert is_version_orderable("") is False + assert is_version_orderable("nightly") is False + assert is_version_orderable("1.0.0") is True + assert is_version_orderable("1.0.0-1", "npm") is True + assert is_version_orderable("987654321098", "docker") is True + + def test_all_numeric_docker_digest_is_a_digest(self) -> None: + """A truncated SHA-256 can be all digits. + + `get_docker_version` truncates to 12 hex chars, so `987654321098` is a + perfectly valid digest. An earlier hex-letter requirement rejected it and + silently dropped real image updates. Shape alone cannot disambiguate a + digest from a calendar version, so the package type decides. + """ + assert is_version_newer("987654321098", "123456789012", "docker") is True + assert is_version_newer("987654321098", "987654321098", "docker") is False + # Without a docker type, a numeric string stays a VERSION and is ordered. + assert is_version_newer("202612180000", "202612190000") is True + + def test_invalid_semver_is_not_ordered(self) -> None: + """SemVer 2.0.0 rules 2, 9 and 10: no leading zeros, no empty identifiers. + + These strings cannot be published to npm, and ordering them fabricated + updates (`1.0.0-01` -> `1.0.0` reported an upgrade). + """ + for invalid in ("1.0.0-01", "01.0.0", "1.0.0-a..b", "1.0.00", "1.0.0-"): + assert is_version_newer(invalid, "1.0.0", "npm") is False, invalid + assert is_version_newer("1.0.0", invalid, "npm") is False, invalid + + def test_semver_spec_precedence_chain(self) -> None: + """The canonical chain from SemVer 2.0.0 spec section 11.4. + + Pins hand-written precedence against the specification rather than + against my own intuition, in both directions. Includes `beta.2` < + `beta.11`, which naive string ordering gets backwards. + """ + chain = [ + "1.0.0-alpha", + "1.0.0-alpha.1", + "1.0.0-alpha.beta", + "1.0.0-beta", + "1.0.0-beta.2", + "1.0.0-beta.11", + "1.0.0-rc.1", + "1.0.0", + ] + for older, newer in zip(chain, chain[1:]): + assert is_version_newer(older, newer, "npm") is True, f"{older} -> {newer}" + assert is_version_newer(newer, older, "npm") is False, f"{newer} -> {older}" + + def test_pypi_keeps_pep440_ordering(self) -> None: + """PyPI publishes PEP 440; post-releases and its prerelease forms hold.""" + assert is_version_newer("1.0.post1", "1.0.post2", "pypi") is True + assert is_version_newer("1.0a1", "1.0", "pypi") is True + + def test_same_digest_in_two_representations_is_not_an_update(self) -> None: + """One image, two spellings, must not read as a new release. + + `get_docker_version` emits a bare truncated digest, but a prefixed or + untruncated form can reach the comparator from a cached value or a + registry that does not truncate. Comparing raw strings called that an + update -- a fabricated notice for an unchanged image. + """ + assert is_version_newer("abcdef123456", "sha256:abcdef123456") is False + assert is_version_newer("sha256:abcdef1234567890", "abcdef123456") is False + # A genuinely different image still registers. + assert is_version_newer("abcdef123456", "fedcba654321") is True + + def test_build_metadata_is_not_a_new_release(self) -> None: + """A local/build-metadata difference is not an update to announce.""" + assert is_version_newer("1.0.0+build.4", "1.0.0+build.5") is False + + def test_prerelease_precedence(self) -> None: + """PEP 440 ordering: a prerelease is OLDER than its release.""" + assert is_version_newer("1.0.0-rc1", "1.0.0") is True + assert is_version_newer("1.0.0", "1.0.0-rc1") is False + class TestGetNpmVersion: """Tests for get_npm_version function.""" diff --git a/uv.lock b/uv.lock index e1107cc3..505504fc 100644 --- a/uv.lock +++ b/uv.lock @@ -1076,6 +1076,7 @@ dependencies = [ { name = "httpx2" }, { name = "jsonschema" }, { name = "mcp" }, + { name = "packaging" }, { name = "pydantic" }, { name = "pyjwt", extra = ["crypto"] }, { name = "python-dotenv" }, @@ -1120,6 +1121,7 @@ requires-dist = [ { name = "jsonschema", specifier = ">=4.20.0,<5.0.0" }, { name = "mcp", specifier = ">=2.0.0,<3.0.0" }, { name = "mypy", marker = "extra == 'dev'", specifier = ">=1.0" }, + { name = "packaging", specifier = ">=22" }, { name = "pmcp", extras = ["dev", "http"], marker = "extra == 'all'" }, { name = "prometheus-client", marker = "extra == 'http'", specifier = ">=0.20" }, { name = "pydantic", specifier = ">=2.0" },