diff --git a/.claude/docs-catalog.json b/.claude/docs-catalog.json index 06f119e3..eb133f2d 100644 --- a/.claude/docs-catalog.json +++ b/.claude/docs-catalog.json @@ -1,6 +1,6 @@ { "version": 1, - "last_updated": "2026-08-23T05:59:17+00:00", + "last_updated": "2026-08-24T12:16:05+00:00", "files": { ".claude/plans/ticklish-inventing-stearns.md": { "purpose": "markdown document", @@ -12,7 +12,8 @@ "P1", "P5", "P2", - "P3B" + "P3B", + "UPDPATH" ] }, "CONTRIBUTING.md": { @@ -27,7 +28,8 @@ "touched_by_phases": [ "P5", "P2", - "P3B" + "P3B", + "UPDPATH" ] }, "SECURITY.md": { @@ -134,6 +136,12 @@ "purpose": "per-phase lane plan", "touched_by_phases": [] }, + "plans/phase-plan-v12-UPDPATH.md": { + "purpose": "per-phase lane plan", + "touched_by_phases": [ + "UPDPATH" + ] + }, "plans/phase-plan-v2-lifecycle.md": { "purpose": "per-phase lane plan", "touched_by_phases": [] @@ -324,7 +332,9 @@ }, "specs/phase-plans-v12.md": { "purpose": "multi-phase roadmap spec", - "touched_by_phases": [] + "touched_by_phases": [ + "UPDPATH" + ] }, "specs/phase-plans-v2.md": { "purpose": "multi-phase roadmap spec", diff --git a/CHANGELOG.md b/CHANGELOG.md index 74ef00e3..96f7a3a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,75 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **A cached description is now checked against the package that is actually + configured, not just against its version.** All three refresh sites — + `refresh_server`'s up-to-date short-circuit, `refresh_all`, and + `check_staleness` — paired a cached entry with a server config by **name** and + then decided freshness by comparing **versions** alone. Nothing asked whether + the cache still described the same *package*, so swapping the configured + package at an equal version served the wrong package's tool descriptions + indefinitely: a cache for `old-pkg@1.0.0` against a config for `new-pkg@1.0.0` + looked current forever. The docker case was already covered — a version + against a digest is `incomparable`, which is not `not_newer` — but a + same-ecosystem swap and an npm ↔ pypi ↔ cargo swap were not. + + Identity is resolved before the comparison at all three sites now. **An + unknown side means "cannot confirm identity", and that resolves to + refresh — never to "cannot compare, so skip the check."** The second phrasing + is the natural one to reach for and is the same fail-open collapse as + `not is_version_newer(...)`, which shipped three times + (Consiliency/pmcp#155, #156, #163) before it was made unrepresentable. + + The cached entry gained a `package_type`, because `package` is a bare name + carrying no ecosystem and npm, pypi and cargo all produce orderable *release* + versions — so npm `foo@1.0.0` and pypi `foo@1.0.0` were indistinguishable to + a name comparison. A cache written before this release has no type, reads as + unknown, and refreshes once. Nothing has to be migrated by hand and the cache + format needs no version bump. + + One cosmetic consequence: a stale report is a `(cached_version, + latest_version)` pair, so an entry that is stale by *identity* at an equal + version prints `srv: 1.0.0 -> 1.0.0`. Confusing to read, but not wrong — that + entry genuinely does need regenerating. +- **`pmcp refresh --check-versions` now honours `--cache-dir`.** `run_refresh` + computed the cache path from `--cache-dir` and then called `check_staleness()` + with no arguments, dropping it, so the check silently inspected the default + cache instead of the one that was asked for. Anyone pointing + `--check-versions` at a non-default cache was reading a different file than + they named, with nothing in the output to say so. + +### Changed +- **`pmcp refresh --check-versions` now reports a server whose package it cannot + look up separately, as unconfirmed rather than as stale.** A server launched + as `node /opt/srv.js` or `python -m thing` has no classifiable package, so its + configured identity is unknown, and "cannot confirm identity" resolves to + refresh. That is the right rule — the only alternative is to read "cannot + classify" as "assume it matches", which is the fail-open reading that let a + swapped package look current in the first place — but it would have made such + a server appear under "servers with newer versions" on every run, permanently, + beneath a `Run 'pmcp refresh --force' to update.` footer that could not settle + it, since the next check still cannot classify the package. So the report is + now split. Servers with a genuinely newer version keep the existing output and + that footer; servers whose current version could not be looked up are listed + under their own heading which says so, notes that this is not the same as + being out of date, and points out that their descriptions are regenerated by + the next plain `pmcp refresh`. Previously these servers were skipped in + silence. A manifest of only registry-installed (npm/pypi/cargo/docker) servers + is unaffected. +- **`refresh_all` now drops an unclassifiable server's cached descriptions when + regeneration fails, where it previously kept them.** A server that fails the + identity gate has its cached entry discarded up front and is regenerated; if + that regeneration then fails — the server does not start, a version lookup + times out — neither the failure fallback nor the final merge puts the old + entry back, and the server is left with no cached descriptions until a later + refresh succeeds. For a `node`/`python` server this costs something real: + there was never *evidence* of a package mismatch, only an inability to confirm + one, so a transient startup failure now costs descriptions that were probably + still accurate. Writing back descriptions that may describe a different + package is the outcome this release exists to prevent, so the trade is + deliberate — but it is a trade, not a free win. + ## [2.3.0] - 2026-08-24 ### Fixed diff --git a/README.md b/README.md index c85b0f6e..e139c549 100644 --- a/README.md +++ b/README.md @@ -479,6 +479,32 @@ URL-mode gateway calls. - `pmcp update ` and `pmcp update --all` call the same gateway update workflow. - Update information is reported on request by `gateway.update_server`; the gateway does not volunteer unprompted "update available" notices. It cannot observe which package version a running server is actually executing, so a volunteered notice could be wrong in either direction (Consiliency/pmcp#150). +**The environment across the update's probe window.** `gateway.update_server` +probes for a new package version and then re-resolves the server config before +restarting anything. Two different kinds of environment change behave +differently across that window, and they are not one rule: + +- A **config-driven** change is **refused**. That means anything resolved into + the server's `env` — which includes every manifest credential, since a + manifest credential is resolved into the server config at load time. If it + changes while the probe is running, `update_server` returns `ok=False` and + does **not** restart: the package is fetched but not activated, and no version + is recorded. The guarantee is that *the config restarted onto is the config + that was probed* (Consiliency/pmcp#151). Your change is not lost — rotate a + credential mid-update and it applies on the **next** update, rather than to a + process that was probed with the old value. +- A **genuinely ambient** variable — one set in the gateway's own environment + rather than in a server's config — is **live at spawn**. It can never cause a + refusal (both sides of the check derive from the same base environment, so an + ambient change affects them equally and cancels out), and the restarted server + receives the value in force when it is spawned, not the one in force when the + update began. This is the same behaviour as `gateway.connect_server`, + `gateway.refresh` and auto-reconnect: every spawn path reads the ambient + environment at spawn time. + +Freezing the ambient environment across an update is deliberately not done +(Consiliency/pmcp#162). + ### Feedback Telemetry - PMCP can emit failure feedback hints and generate GitHub issue payload previews for agents. @@ -1412,6 +1438,9 @@ pmcp logs --tail 100 # Last 100 lines pmcp refresh pmcp refresh --server github # Refresh specific server pmcp refresh --force # Force reconnect all +pmcp refresh --check-versions # Report stale cached descriptions without refreshing + # (honours --cache-dir; a local server whose package + # cannot be classified reports stale on every run) # Initialize config (interactive) pmcp init diff --git a/plans/phase-plan-v12-UPDPATH.md b/plans/phase-plan-v12-UPDPATH.md index 8a2e6d28..5f7bc69b 100644 --- a/plans/phase-plan-v12-UPDPATH.md +++ b/plans/phase-plan-v12-UPDPATH.md @@ -2,7 +2,7 @@ phase_loop_plan_version: 1 phase: UPDPATH roadmap: specs/phase-plans-v12.md -roadmap_sha256: 3b6439a9dddb9ef5b5b709ed6fa32f889a082ed0b0118554e6ae9c2265bf4847 +roadmap_sha256: 2015eca64695daef2a342a772e538a93f3e6f89dcb08840275bd541d9f06a84a --- # PHASE-2-UPDPATH: Update-path identity and environment contracts @@ -367,3 +367,68 @@ automation: deliberately rotated; refusing would fail an update the operator wanted. A lane that finds the decision uncomfortable must stop and report rather than re-deciding it mid-execution. + +## Post-execution amendments (2026-08-24) + +Recorded by SL-3 after SL-1 and SL-2 merged. The full list of what execution +discovered — including two operator-visible consequences this plan did not +anticipate — is in `specs/phase-plans-v12.md` under +"Post-execution amendments — UPDPATH (2026-08-24)". Only the points where **this +document** was wrong or imprecise are repeated here. + +- **"Resolve identity before the short-circuit" was implemented as "resolve + before, compare inside."** The `detect_package_type` / `pkg_name` resolution + moved above the early return as this plan required, and exactly one call + remains in `refresh_server`. The `_same_package` **call**, however, is the + second conjunct of the existing `if version and … and compare_versions(…) == + "not_newer"`, so `get_package_version` still runs before identity is + consulted. SL-1 reported that placing the guard ahead of the fetch broke + `TestUpToDateShortCircuit`'s call-count assertion, since a mismatch would then + skip the version lookup entirely. This plan read as though the guard would sit + ahead of the fetch; it does not, and nothing in EC-UPDPATH-1..3 requires it to. +- **The `TestUpToDateShortCircuit` fixture warning was misplaced.** That class + needed no changes at all: both of its tests assert the short-circuit does + *not* fire, so an extra always-`False` conjunct cannot move their outcome — + which also means neither of them discriminates a degenerate gate. The + prediction was right for `TestCheckStaleness` (three fixtures gained + `package_type="npm"`), and the test this plan correctly identified as the one + that catches an always-`False` gate, + `test_short_circuit_is_a_single_compare_versions_call`, lives in + `TestShortCircuitUsesCompareVersions`, not in `TestUpToDateShortCircuit`; its + fixture needed `package_type="npm"` too. All four were pure insertions, so + `git diff --numstat -- tests/` reports zero deletions and the fixture work is + invisible unless you read the hunks. +- **EC-3 needed state, not just a gated pair.** This plan named both of + `refresh_all`'s failure paths correctly but not what closing them costs: + setting `existing = None` stops the callee from short-circuiting and does + nothing about the final merge loop, which re-adds cached entries straight from + `existing_servers`. `refresh_all` keeps a `mismatched: set[str]` and excludes + those names from the merge. +- **EC-7 needed no code path of its own** — the empty-`package` case falls out of + `_same_package`'s any-unknown arm for free. +- **`roadmap_sha256` was refreshed at closeout**, and this is a deliberate change + of convention. This phase's own docs lane amended the roadmap, so the pin + written at planning time no longer matched and `validate_plan_doc` reported + `(FM) roadmap_sha256 does not match the referenced roadmap` as an **error**. + + The pre-merge board split on it. The red-team seat called it blocking: the + validator is authoritative and prior stale pins do not override the contract. + The adversarial and correctness seats ruled the opposite — the field is + *provenance*, recording what the plan was planned against, and re-pinning + falsifies it, since this plan was demonstrably **not** planned against a + roadmap whose amendment block contains this plan's own closeout. The + correctness seat further noted that the phase-loop runtime deliberately + refuses to gate live behaviour on this hash, treating drift as + "not-corroborated" rather than an error. + + Both readings are defensible; the maintainer chose to re-pin, so a shipped + artifact validates clean rather than normalising a validator error across + three plans. The planning-time binding is not lost — it is recoverable from + this file's own history. The v12 plans that ran before this one (TRISTATE, + FANOUT) still carry stale pins; UPDPATH is the first to refresh at closeout, + and that is the convention going forward. + + Ordering matters and is easy to get wrong: the re-pin must be the **last** + step before merge. Computing it any earlier goes stale the moment a docs lane + or a board fix touches the roadmap again — which happened twice during this + phase's closeout. diff --git a/specs/phase-plans-v12.md b/specs/phase-plans-v12.md index 58d71105..8f0d305f 100644 --- a/specs/phase-plans-v12.md +++ b/specs/phase-plans-v12.md @@ -149,6 +149,173 @@ Close the two remaining correctness gaps in the update path: a refresh short-cir **Produces** - (none) +### Post-execution amendments — UPDPATH (2026-08-24) + +- **EC-UPDPATH-7 needed no code path of its own.** The empty-`package` case falls + out of `_same_package` for free: the predicate rejects any of its four + arguments being `None`, `""` or `"unknown"` in one loop, and + `load_descriptions_cache` renders an absent `package` as `""`, so the same arm + that covers an absent `package_type` covers it. There is no empty-package + branch anywhere in `refresher.py`, and the criterion is pinned by + `TestUnknownPackageForcesRefresh::test_empty_cached_package_forces_refresh` + rather than by dedicated code. +- **The gate landed *inside* the short-circuit conjunction, after the version + fetch — not strictly before it.** EC-UPDPATH-2 is satisfied in the sense that + matters: the `detect_package_type` / `pkg_name` resolution moved above the + early return (exactly one call remains in `refresh_server`), so the comparison + can consult identity at all. But the `_same_package` call is the second + conjunct of the existing `if version and … and compare_versions(…) == + "not_newer"`, so `get_package_version` still runs first on every non-forced + path. SL-1 reported that gating *before* the fetch broke + `TestUpToDateShortCircuit`'s call-count assertion — an identity mismatch would + short-circuit past the fetch entirely and the version lookup would never + happen. As placed, the fetch is not skipped and no existing assertion moved. + A future lane that wants to save the fetch on a mismatch must expect that test + to need rewriting, not just the guard to move. +- **Two operator-visible consequences the plan did not anticipate.** Both are in + the CHANGELOG under `[Unreleased]`: + 1. `pmcp refresh --check-versions` now reports an **unclassifiable local + server** (`node /opt/srv.js`, `python -m thing`) as stale on **every** run, + printing `local: 1.0.0 -> unknown` and the "Run 'pmcp refresh --force' to + update." footer permanently — a refresh does not settle it, because the + next check still cannot classify the package. Such servers were previously + skipped in silence. This follows unavoidably from "cannot confirm + identity → refresh"; the plan chose that semantics deliberately but did not + trace it to this output. + 2. `refresh_all` now **drops** an unclassifiable server's cached entry when + regeneration fails, where it previously retained it. The plan explicitly + sanctioned dropping on failure ("dropping the entry on a failed + regeneration is acceptable"), but reasoned about it only for a *confirmed* + mismatch. For a `node`/`python` server there is no evidence of a mismatch, + only an inability to confirm one, so a transient startup failure now costs + cached descriptions that were probably still accurate. +- **EC-UPDPATH-3 needed a `refresh_all`-local record, not just a gated pair.** + Setting `existing = None` for a mismatched server stops the callee from + short-circuiting, but does nothing about the final merge loop, which re-adds + every cached entry missing from the new set straight from `existing_servers`. + `refresh_all` therefore keeps a `mismatched: set[str]` and excludes those names + from the merge. The plan named both failure paths correctly; what it did not + say is that closing them requires state the gate itself cannot carry. +- **The plan's fixture-update prediction was right, and is easy to misread as + wrong.** `git diff --numstat main..HEAD -- tests/` shows **zero** deleted + lines, which looks like no existing test was touched. Four pre-existing + fixtures did need `package_type="npm"` added — three in `TestCheckStaleness` + (as the plan predicted) and one in + `TestShortCircuitUsesCompareVersions::test_short_circuit_is_a_single_compare_versions_call` + (which the plan named but listed under a different class) — and every one of + them was a pure insertion. `TestUpToDateShortCircuit` stayed **green** with no + fixture change: both of its tests assert the short-circuit does *not* fire, so + an extra always-`False` conjunct cannot change their outcome. That also means + those two tests do **not** discriminate a degenerate gate; the one that does + is `test_short_circuit_is_a_single_compare_versions_call`'s `result is + existing`, exactly as the plan said. +- **Correction (pre-merge board): "needed nothing" was true for staying green + and false for staying load-bearing — the phase hollowed out two pre-existing + regression guards.** `TestUpToDateShortCircuit`'s fixtures carried no + `package_type`, so under the new gate `_same_package` refused on the unknown + type and the short-circuit's `and` chain **never reached + `compare_versions`**. Both tests still passed, but for the identity reason, + pinning nothing about the version comparison they exist to guard — the + unorderable/incomparable fail-open of Consiliency/pmcp#156 and #164. + + Demonstrated, not argued: rewriting the short-circuit to + `compare_versions(...) != "newer"` — so an `incomparable` pair reads as up to + date, which is precisely that fail-open — left the **entire suite green** + (172 passed across the two files). A phase whose purpose is to close a + fail-open had silently removed the only coverage of a different one, and + neither the lane, the orchestrator, nor the plan-stage board caught it; the + adversarial seat of the pre-merge board did. + + Fixed by making both fixtures confirm identity so the version path is + actually exercised: `package_type="npm"` for the unorderable case, and the + incomparable case rebuilt as a **same-ecosystem** docker pair (a cached tag + against a fetched digest) — the original npm-cache-vs-docker-config pairing is + now refused on ecosystem alone by the very gate this phase added, so it could + never have reached `compare_versions` again. That cross-ecosystem case keeps + its own coverage in `TestPackageIdentityGate`. Both tests now fail under the + mutant and pass without it. + + **The general lesson, which outlives this phase:** adding an early conjunct to + an existing condition can silently retire every test downstream of it. Those + tests keep passing, so nothing signals the loss. When a gate is inserted ahead + of an existing check, every test that reached the old check through the new + gate's position must be re-proved load-bearing by mutation, not by re-running + it green. +- **A third write site for package identity was missed until the pre-merge + board.** `gateway.update_server` mutates the cached entry in place + (`tools/handlers.py`), setting `version`, `tools` and `generated_at` but not + `package` or `package_type`. Two consequences, both closed here. A legacy + entry passing through `update_server` stayed permanently unverifiable even + though `update_server` had just classified its package. Worse, the entry could + end up carrying the **new** package's version and live tool list under the + **old** package's label — after which `_same_package` would *confirm* identity + against the stale label and the short-circuit would serve the wrong package's + descriptions, which is the exact defect this phase exists to close, reachable + in three operator steps (swap the config, `update_server`, revert). Fixed by + writing `package` and `package_type` alongside the version, pinned by + `test_update_server_relabels_the_entry_with_the_package_it_describes`. The + lane's audit found two write sites and the plan repeated it; the count was + three. +- **The fix for the third write site was itself a hollow pin, and a second + board round caught it.** `test_update_server_relabels_the_entry_...` asserted + `entry.package == "@playwright/mcp"` against a fixture whose package was + *already* `@playwright/mcp` — the value `detect_package_type` returns there — + so the assertion held whether or not the assignment ran. Deleting + `desc_entry.package = package_name` left it green. Two seats found this + independently, one commit after this very amendment recorded the lesson about + green not being evidence. The test now starts the entry under a different + package, and each assignment is pinned by its own mutant. +- **Relabelling turned out to be the wrong repair; the entry is now dropped + instead.** `update_server` deliberately preserves `capability_summary` on a + version bump, on the reasoning that it only feeds startup MCP-instructions + text and being "one refresh cycle behind" is a small, non-functional gap. + That reasoning does not survive a relabel: once `package` **and** `version` + both name the new package, the freshness short-circuit CONFIRMS identity and + never regenerates the entry, so the old package's summary would be served + under the new package's label *permanently*. A genuine package change now + deletes the entry and lets the next refresh rebuild it wholesale — this + phase's own rule applied to itself: cannot confirm the cache describes this + package → refresh, never patch around it. A same-package version bump is + unchanged and still backfills `package_type`. +- **A known limitation, filed rather than fixed: Consiliency/pmcp#180.** + `detect_package_type` collapses distinct packages for some legal command + forms — `registry:5000/old-image` and `registry:5000/new-image` both resolve + to `("docker", "registry")`, and `npm exec old-pkg` / `new-pkg` both to + `("npm", "exec")`. For those forms `_same_package` confirms identity across a + real swap, so EC-UPDPATH-1/3/6 do **not** hold there. The defect is + pre-existing and only became consequential when this phase started comparing + package names; `detect_package_type` backs every version lookup in the + codebase, so re-parsing it has blast radius well beyond this phase and was + deliberately deferred to its own plan and review round. +- **The CLI split's group boundary was unpinned.** The test asserting a swap is + reported checked only that the server name appeared in the output — and the + name appears in *both* groups, so it passed whichever group the entry landed + in. A classifiable identity mismatch at an equal version is actionable + (`--force` settles it) and must sit in the stale group *with* the remedy; + routing it to the "could not confirm" group left the suite green. Now pinned + on the rendered line and the remedy's presence. +- **EC-UPDPATH-4 was docstring-only, as planned, and the docstring is now pinned + by a test.** `handlers.py`'s diff for this phase is the `update_server` + docstring and nothing else. `test_update_server_docstring_states_both_probe_window_env_contracts` + asserts both halves are stated, so an edit that collapses them back into a + single "live environment" sentence fails the suite rather than passing review. +- **`docs/` was not created.** SL-3 owned `docs/**` and deliberately left it + absent; the probe-window environment contract landed in `README.md` under + "Subordinate MCP Updates", which is where a user will actually meet it. +- **The docs-catalog rescan helper is absent on this host and its history is + already incomplete for v12.** `_shared/scaffold_docs_catalog.py` does not + exist, so SL-3 audited `.claude/docs-catalog.json` by hand. That was the right + outcome independently: the helper scans a fixed set of roots plus a + `KNOWN_FILES` list that does **not** include `SPEC_COMPLIANCE.md`, so a rescan + would have dropped that root-level entry — Consiliency/pmcp#171, confirmed + against the helper's source rather than assumed. Separately, both earlier v12 + phases left `touched_by_phases` empty on the files they edited (TRISTATE and + FANOUT are absent from `CHANGELOG.md`'s list, and `specs/phase-plans-v12.md` + carried an empty list despite holding FANOUT's own amendment block), so that + field is not a reliable history for v12. UPDPATH's aliases were added; the + missing ones were **not** backfilled, because there is no way to verify from + the catalog alone which files each phase actually touched. + ### Phase 3 — Downstream catalog reconciliation and fan-out (FANOUT) **Objective** diff --git a/src/pmcp/cli.py b/src/pmcp/cli.py index 4f3b44f5..09c3dd43 100644 --- a/src/pmcp/cli.py +++ b/src/pmcp/cli.py @@ -757,6 +757,14 @@ def parse_args() -> argparse.Namespace: return parser.parse_args() +# `check_staleness` reports each stale server as `(cached_version, +# latest_version or "unknown")`, so this literal is what a package it could not +# resolve surfaces as in the second slot. It is deliberately not +# `refresher._UNIDENTIFIED`: that describes the identity *fields*, a different +# axis from the latest-version slot read here. +_UNRESOLVED_VERSION = "unknown" + + async def run_refresh(args: argparse.Namespace) -> None: """Run the refresh command.""" from pmcp.manifest.refresher import ( @@ -773,15 +781,53 @@ async def run_refresh(args: argparse.Namespace) -> None: if args.check_versions: # Just check for updates logger.info("Checking for package version updates...") - stale = await check_staleness() + stale = await check_staleness(cache_path=cache_path) + + # One dict, two things to tell an operator. An entry whose latest + # version came back `_UNRESOLVED_VERSION` is one pmcp could not look up + # at all -- a server launched as `node /opt/srv.js` has no package to + # resolve -- so its identity can never be confirmed and it would be + # listed here on every run, permanently, under a `--force` remedy that + # cannot settle it. `check_staleness` is right to include it (an + # unconfirmed identity must resolve to refresh, never to "skip the + # check"); it is the report that has to keep the two apart. + outdated = { + name: pair for name, pair in stale.items() if pair[1] != _UNRESOLVED_VERSION + } + unresolved = { + name: pair for name, pair in stale.items() if pair[1] == _UNRESOLVED_VERSION + } if not stale: print("All cached descriptions are up to date.") - else: - print(f"Found {len(stale)} servers with newer versions:") - for name, (old, new) in stale.items(): + + if outdated: + print(f"Found {len(outdated)} servers with newer versions:") + for name, (old, new) in outdated.items(): print(f" {name}: {old} -> {new}") print("\nRun 'pmcp refresh --force' to update.") + + if unresolved: + if outdated: + print() + print( + f"Found {len(unresolved)} servers whose version could not be confirmed:" + ) + for name, (old, _) in unresolved.items(): + print(f" {name} (cached at {old})") + print( + "\npmcp could not look up a current version for these, so it " + "cannot tell whether" + ) + print( + "their cached descriptions still match what the server runs. " + "That is not the" + ) + print( + "same as being out of date. Their descriptions are " + "regenerated by the next" + ) + print("'pmcp refresh'.") return # Refresh descriptions diff --git a/src/pmcp/manifest/refresher.py b/src/pmcp/manifest/refresher.py index a9b7c3aa..9fc1b1f8 100644 --- a/src/pmcp/manifest/refresher.py +++ b/src/pmcp/manifest/refresher.py @@ -68,6 +68,9 @@ def load_descriptions_cache(cache_path: Path | None = None) -> DescriptionsCache tools = [PrebuiltToolInfo(**t) for t in server_data.get("tools", [])] servers[name] = GeneratedServerDescriptions( package=server_data.get("package", ""), + # Default to None, not "": an absent value must stay + # distinguishable from a recorded empty one. + package_type=server_data.get("package_type"), version=server_data.get("version", ""), generated_at=server_data.get("generated_at", ""), capability_summary=server_data.get("capability_summary", ""), @@ -101,6 +104,7 @@ def save_descriptions_cache( "servers": { name: { "package": desc.package, + "package_type": desc.package_type, "version": desc.version, "generated_at": desc.generated_at, "capability_summary": desc.capability_summary, @@ -186,6 +190,45 @@ def _infer_risk(tool_name: str, description: str) -> str: return "low" +# A package name or type that carries no identity information. `detect_package_type` +# reports `"unknown"` for a command it cannot classify, and a cache written before +# `package_type` existed reports `None`. +_UNIDENTIFIED = ("", "unknown") + + +def _same_package( + cached_package: str, + cached_type: str | None, + configured_package: str | None, + configured_type: str | None, +) -> bool: + """Whether a cached entry and a configured server describe the same package. + + Cached descriptions are paired with a server config by NAME, and freshness is + then decided by comparing versions. That answers "is the cache old" but not + "does the cache still describe this package", so a swap at an equal version + -- `old-pkg@1.0.0` cached against `new-pkg@1.0.0` configured -- serves the + wrong package's tool descriptions indefinitely. + + **An unknown side means "cannot confirm identity", which resolves to + refresh -- never to "cannot compare, so skip the check."** Any of the four + values being empty, `None` or `"unknown"` therefore returns `False`, and at + every call site `False` means the freshness short-circuit does not fire. + The second reading is the natural one to reach for and is the same + fail-open collapse as `not is_version_newer(...)`, which shipped three + times (Consiliency/pmcp#155, #156, #163). + + The type arms are not decorative: `package` is a bare name with no + ecosystem, so npm `foo` and pypi `foo` are indistinguishable without it, + and npm/pypi/cargo all produce orderable *release* versions so + `compare_versions` never reports them `incomparable`. + """ + for value in (cached_package, cached_type, configured_package, configured_type): + if value is None or value in _UNIDENTIFIED: + return False + return cached_package == configured_package and cached_type == configured_type + + async def refresh_server( server_config: ServerConfig, existing_cache: GeneratedServerDescriptions | None = None, @@ -209,16 +252,32 @@ async def refresh_server( server_name = server_config.name + # Determine package info. This MUST precede the freshness short-circuit + # below: the short-circuit cannot consult identity unless identity has + # already been resolved, which is why this resolution moved up here rather + # than being duplicated (Consiliency/pmcp#178, EC-UPDPATH-2). + pkg_type, pkg_name = detect_package_type(server_config.command, server_config.args) + if not pkg_name: + pkg_name = f"{server_config.command} {' '.join(server_config.args)}" + # Check for staleness if we have cached data if existing_cache and not force: # Get current package version - version, pkg_type = await get_package_version( + version, fetched_type = await get_package_version( server_config.command, server_config.args ) + # Identity before freshness: a version comparison is only meaningful + # once the cache is known to describe the package that is configured. if ( version - and compare_versions(existing_cache.version, version, pkg_type) + and _same_package( + existing_cache.package, + existing_cache.package_type, + pkg_name, + pkg_type, + ) + and compare_versions(existing_cache.version, version, fetched_type) == "not_newer" ): logger.debug( @@ -228,11 +287,6 @@ async def refresh_server( logger.info(f"Refreshing descriptions for {server_name}...") - # Determine package info - pkg_type, pkg_name = detect_package_type(server_config.command, server_config.args) - if not pkg_name: - pkg_name = f"{server_config.command} {' '.join(server_config.args)}" - # Get version version, _ = await get_package_version(server_config.command, server_config.args) version = version or "unknown" @@ -283,6 +337,7 @@ async def refresh_server( return GeneratedServerDescriptions( package=pkg_name, + package_type=pkg_type, version=version, generated_at=datetime.now(timezone.utc).isoformat(), capability_summary=capability_summary, @@ -342,6 +397,16 @@ async def refresh_all( new_servers: dict[str, GeneratedServerDescriptions] = {} semaphore = asyncio.Semaphore(REFRESH_VERSION_LOOKUP_CONCURRENCY) + # Targeted servers whose cached entry does not describe the package the + # manifest now configures. `refresh_all` assembles the (cached, configured) + # pair itself, so it must apply the gate rather than rely on the callee, and + # such an entry must not come back through EITHER failure path below: the + # None/raise fallback returning it as a live result, or the merge loop at + # the end writing it straight back to disk. Dropping the entry when + # regeneration fails is acceptable; writing the wrong package's + # descriptions back is not. + mismatched: set[str] = set() + async def refresh_target( name: str, ) -> tuple[str, GeneratedServerDescriptions | None]: @@ -351,6 +416,20 @@ async def refresh_target( return name, None existing = existing_servers.get(name) + if existing is not None: + cfg_type, cfg_name = detect_package_type( + server_config.command, server_config.args + ) + if not _same_package( + existing.package, existing.package_type, cfg_name, cfg_type + ): + logger.info( + f"Cached descriptions for {name} do not confirm the " + f"configured package (cached {existing.package!r}); " + "discarding them and regenerating" + ) + mismatched.add(name) + existing = None try: async with semaphore: @@ -372,9 +451,11 @@ async def refresh_target( if result: new_servers[name] = result - # Merge with existing cache (keep servers not in target list) + # Merge with existing cache (keep servers not in target list). A targeted + # entry that failed the identity gate is deliberately absent from + # `new_servers`; re-adding it here would undo the gate. for name, desc in existing_servers.items(): - if name not in new_servers: + if name not in new_servers and name not in mismatched: new_servers[name] = desc # Create new cache @@ -415,10 +496,27 @@ async def check_staleness( if not server_config: continue + cfg_type, cfg_name = detect_package_type( + server_config.command, server_config.args + ) + version, pkg_type = await get_package_version( server_config.command, server_config.args ) + # NOTE the inverted polarity relative to `refresh_server`. There, a + # match plus `not_newer` SKIPS a refresh, so an identity mismatch + # withholds the skip. Here, `newer` ADDS to the stale dict, so an + # identity mismatch must ADD -- porting that guard mechanically would + # report a swapped package as up to date, which is the defect. + if not _same_package(desc.package, desc.package_type, cfg_name, cfg_type): + stale_servers[name] = (desc.version, version or "unknown") + logger.info( + f"Server {name} is stale: cached package {desc.package!r} does " + f"not confirm the configured package {cfg_name!r}" + ) + continue + if version and compare_versions(desc.version, version, pkg_type) == "newer": stale_servers[name] = (desc.version, version) logger.info(f"Server {name} is stale: {desc.version} -> {version}") diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index b09e072f..fc946c98 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -4940,7 +4940,36 @@ async def submit_feedback(self, input_data: dict[str, Any]) -> SubmitFeedbackOut ) async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: - """gateway.update_server - Update a subordinate MCP package and restart it.""" + """gateway.update_server - Update a subordinate MCP package and restart it. + + The probe below is a 60-second await, and the config is re-resolved + after it before anything restarts. Two DIFFERENT things happen to the + child environment across that window, and they must not be described as + one: + + 1. A CONFIG-DRIVEN environment change is REFUSED. This includes every + manifest credential -- ``_manifest_server_to_config`` resolves it + into ``LocalMcpServerConfig.env`` at load time, so rotating it during + the probe makes the recheck's child environment differ from the + probe's and this tool returns ``ok=False`` without restarting: the + package is fetched but NOT activated and no version is recorded. The + guarantee is "the config restarted onto is the config that was + probed" (Consiliency/pmcp#151), so the operator's rotation applies on + the next update rather than to a process that was probed with the old + value. + + 2. A GENUINELY AMBIENT variable is LIVE at spawn. Both sides of the + guard below derive from ONE ``stripped_base``, so an ambient change + affects them equally and cancels out -- it can never cause a refusal. + ``sanitized_subprocess_env`` then calls ``os.environ.copy()`` at + spawn time, the same read ``connect``, ``refresh`` and + auto-reconnect reach, so the restarted server gets the value in force + when it is spawned, not the one in force when the update began. + + Freezing the ambient environment across the update is deliberately NOT + done here; it would mean threading a frozen env through ClientManager, + which is a separate concern from this TOCTOU. + """ parsed = UpdateServerInput.model_validate(input_data) server_name = parsed.server_name @@ -5204,39 +5233,72 @@ def _child_env(candidate: ResolvedServerConfig) -> dict[str, str]: and server_name in self._descriptions_cache.servers ): desc_entry = self._descriptions_cache.servers[server_name] - desc_entry.version = latest_version - # The restart just gave us a live, freshly-connected tool - # list for this server -- regenerate `tools`/`generated_at` - # from it too, not just `version`. Otherwise the entry - # would claim tools were "generated" for a version they - # weren't (GeneratedServerDescriptions.version is - # documented as "Package version when generated"), and a - # package update that changed tool signatures/descriptions - # or added/removed tools would keep serving the pre-update - # tool list to offline catalog_search. This regenerates - # from the connection `restart_server` already made -- - # no extra stdio round-trip, unlike refresher.refresh_server. - # `capability_summary` is deliberately left as-is: it only - # feeds GatewayServer.initialize()'s startup MCP-instructions - # text (computed once, not read live by catalog_search or - # invoke), so leaving it one refresh cycle behind is a much - # smaller, non-functional gap than a stale `tools` list was. - live_tools = [ - t - for t in self._client_manager.get_all_tools() - if t.server_name == server_name - ] - desc_entry.tools = [ - PrebuiltToolInfo( - name=t.tool_name, - description=t.description, - short_description=t.short_description, - tags=t.tags, - risk_hint=t.risk_hint.value, - ) - for t in live_tools - ] - desc_entry.generated_at = datetime.now(timezone.utc).isoformat() + if desc_entry.package != package_name: + # A genuine PACKAGE change, not an update of this + # entry. Every field describes a different package, so + # patching them one by one leaves a permanently MIXED + # entry: `capability_summary` is deliberately preserved + # on a version bump (see below), but once `package` and + # `version` are both rewritten the freshness + # short-circuit CONFIRMS identity and never regenerates + # the entry -- so the old package's summary would be + # served under the new package's label forever, not + # merely one refresh cycle behind. Drop it and let the + # next refresh rebuild it wholesale: this phase's own + # rule applied here -- cannot confirm the cache + # describes this package -> refresh, never patch around + # it (ah board review, red-team seat). + del self._descriptions_cache.servers[server_name] + else: + desc_entry.version = latest_version + # Re-label the entry with the package it now describes, not + # just the version. This is a THIRD write site for package + # identity alongside refresh_server's constructor and + # save_descriptions_cache's dict, and omitting it re-opens + # the exact defect the identity gate closes: an entry whose + # `package` still names the OLD package while its version + # and tools come from the NEW one. `_same_package` would + # then CONFIRM identity against the stale label and the + # short-circuit would serve the new package's descriptions + # under the old package's name. It also strands a legacy + # entry as permanently unverifiable, since `package_type` + # would stay None even though we have just classified it. + # Both are in scope from detect_package_type above, which + # already refused an unclassifiable package. + desc_entry.package = package_name + desc_entry.package_type = package_type + # The restart just gave us a live, freshly-connected tool + # list for this server -- regenerate `tools`/`generated_at` + # from it too, not just `version`. Otherwise the entry + # would claim tools were "generated" for a version they + # weren't (GeneratedServerDescriptions.version is + # documented as "Package version when generated"), and a + # package update that changed tool signatures/descriptions + # or added/removed tools would keep serving the pre-update + # tool list to offline catalog_search. This regenerates + # from the connection `restart_server` already made -- + # no extra stdio round-trip, unlike refresher.refresh_server. + # `capability_summary` is deliberately left as-is: it only + # feeds GatewayServer.initialize()'s startup MCP-instructions + # text (computed once, not read live by catalog_search or + # invoke), so leaving it one refresh cycle behind is a much + # smaller, non-functional gap than a stale `tools` list was. + live_tools = [ + t + for t in self._client_manager.get_all_tools() + if t.server_name == server_name + ] + desc_entry.tools = [ + PrebuiltToolInfo( + name=t.tool_name, + description=t.description, + short_description=t.short_description, + tags=t.tags, + risk_hint=t.risk_hint.value, + ) + for t in live_tools + ] + desc_entry.generated_at = datetime.now(timezone.utc).isoformat() if self._descriptions_cache_path is not None: try: save_descriptions_cache( diff --git a/src/pmcp/types.py b/src/pmcp/types.py index 966a975f..5f31d1ae 100644 --- a/src/pmcp/types.py +++ b/src/pmcp/types.py @@ -1360,6 +1360,13 @@ class GeneratedServerDescriptions(BaseModel): """Pre-generated descriptions for a single server.""" package: str # e.g., "@playwright/mcp" + # Ecosystem the package name belongs to, e.g. "npm". A bare name cannot + # express package identity -- npm `foo` and pypi `foo` are different + # packages that both produce orderable release versions -- so the identity + # gate needs the type. `None` means the entry never recorded one (every + # cache written before this field existed), which reads as unknown and + # therefore forces a refresh. + package_type: str | None = None version: str # Package version when generated generated_at: str # ISO timestamp capability_summary: str # L1: For MCP instructions diff --git a/tests/test_cli.py b/tests/test_cli.py index f9c8c1b5..77ba443f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9,8 +9,10 @@ from unittest.mock import AsyncMock, patch import pytest +import yaml from pmcp.cli import async_main, parse_args, setup_logging +from pmcp.manifest.loader import Manifest, ServerConfig class TestParseArgs: @@ -2240,3 +2242,391 @@ async def test_status_json_with_data( output = json.loads(captured.out) assert "servers" in output assert "tools" in output or "total_tools" in output + + +class TestCheckVersionsIdentityGate: + """`pmcp refresh --check-versions` end to end (EC-UPDPATH-6). + + Two defects meet here. `run_refresh` computes + `cache_path = get_cache_path(args.cache_dir)` and then calls + `check_staleness()` with no arguments, so `--cache-dir X` silently inspects + the DEFAULT cache instead of `X`. And `check_staleness` compares versions + only, so a cache for one package against a config for another at the same + version prints "All cached descriptions are up to date." + + The cache path is deliberately NOT monkeypatched -- pointing the CLI at a + fixture cache through its own flag is the whole point of the test, and a + test that reached past the CLI to patch it would be a unit test dressed as + a CLI test. `get_package_version` is patched because it shells out to the + network; `load_manifest` is patched so the assertion does not depend on + whichever manifest overlays happen to exist on the host. + """ + + @staticmethod + def _manifest() -> Manifest: + return Manifest( + version="1.0", + cli_alternatives={}, + servers={ + "srv": ServerConfig( + name="srv", + description="", + keywords=[], + install={}, + command="npx", + args=["-y", "new-pkg"], + ) + }, + discovery_queue_path=".mcp-gateway/discovery_queue.json", + ) + + @staticmethod + def _write_cache(cache_dir: Path) -> None: + cache_dir.mkdir(parents=True, exist_ok=True) + (cache_dir / "descriptions.yaml").write_text( + yaml.safe_dump( + { + "generated_at": "2025-01-01T00:00:00Z", + "gateway_version": "1.0.0", + "servers": { + "srv": { + "package": "old-pkg", + "package_type": "npm", + "version": "1.0.0", + "generated_at": "2025-01-01T00:00:00Z", + "capability_summary": "stale old-pkg summary", + "tools": [], + } + }, + } + ) + ) + + @pytest.mark.asyncio + async def test_check_versions_reads_the_requested_cache_dir( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + # chdir so the DEFAULT cache location resolves under an empty tmp dir: + # without this the pre-fix read could find a real cache in the repo. + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "fixture-cache" + self._write_cache(cache_dir) + + with patch( + "sys.argv", + [ + "pmcp", + "refresh", + "--check-versions", + "--cache-dir", + str(cache_dir), + ], + ): + args = parse_args() + + with patch( + "pmcp.manifest.refresher.load_manifest", return_value=self._manifest() + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new=AsyncMock(return_value=("1.0.0", "npm")), + ): + await async_main(args) + + out = capsys.readouterr().out + assert "All cached descriptions are up to date." not in out, ( + "--check-versions inspected the default cache instead of " + "--cache-dir, so the fixture cache was never read" + ) + + @pytest.mark.asyncio + async def test_check_versions_does_not_report_up_to_date_across_a_swap( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "fixture-cache" + self._write_cache(cache_dir) + + with patch( + "sys.argv", + [ + "pmcp", + "refresh", + "--check-versions", + "--cache-dir", + str(cache_dir), + ], + ): + args = parse_args() + + with patch( + "pmcp.manifest.refresher.load_manifest", return_value=self._manifest() + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new=AsyncMock(return_value=("1.0.0", "npm")), + ): + await async_main(args) + + out = capsys.readouterr().out + assert "srv" in out, ( + "a cache for old-pkg against a config for new-pkg was not " + f"reported at all (output: {out!r})" + ) + # Presence alone is not enough: BOTH report groups contain the server + # name, so `"srv" in out` passes whichever group it lands in. A + # classifiable identity mismatch at an equal version is ACTIONABLE -- + # `--force` regenerates it -- so it belongs in the stale group WITH the + # remedy, not in the "could not confirm" group where the remedy is + # deliberately withheld. Verified by mutation: routing an equal-version + # pair into the unverifiable group leaves the whole suite green without + # these two assertions (ah board review, correctness seat). + assert "srv: 1.0.0 -> 1.0.0" in out, ( + "an identity mismatch at an equal version must be reported in the " + f"stale group with its versions (output: {out!r})" + ) + assert "pmcp refresh --force" in out, ( + "the stale group must keep the remedy that actually settles it " + f"(output: {out!r})" + ) + + +class TestCheckVersionsUnverifiableDisplay: + """`pmcp refresh --check-versions` splits its report in two (EC-UPDPATH-7). + + `check_staleness` returns one dict, but it holds two different things to + say to an operator. A server with a real newer version is actionable: + `pmcp refresh --force` settles it. A server whose latest version came back + `"unknown"` is not: for `node /opt/srv.js` there is no package to look up, + so the identity can never be confirmed, and the entry would be listed as + stale on every run forever under a remedy that cannot settle it. + + The dict is the contract and it is correct -- the fix is at the point of + display, so these drive `async_main` end to end rather than asserting on + `check_staleness`. Fixtures follow `TestCheckVersionsIdentityGate`: + `load_manifest` is patched so the assertions do not depend on whichever + manifest overlays exist on the host, and `get_package_version` is patched + because it shells out to the network. + """ + + NEWER_HEADING = "servers with newer versions" + FORCE_FOOTER = "refresh --force" + UNVERIFIABLE_HEADING = "could not be confirmed" + + @staticmethod + def _manifest(*names: str) -> Manifest: + """A manifest of *names* drawn from a fixed two-server cast. + + `pkg` is classifiable (npx -> npm) and gets a real version back. + `local` is not: `node` classifies as `("unknown", None)`, which is the + whole subject of these tests. + """ + cast = { + "pkg": ServerConfig( + name="pkg", + description="", + keywords=[], + install={}, + command="npx", + args=["-y", "some-pkg"], + ), + "local": ServerConfig( + name="local", + description="", + keywords=[], + install={}, + command="node", + args=["/opt/srv.js"], + ), + } + return Manifest( + version="1.0", + cli_alternatives={}, + servers={name: cast[name] for name in names}, + discovery_queue_path=".mcp-gateway/discovery_queue.json", + ) + + @staticmethod + def _write_cache(cache_dir: Path, **entries: str) -> None: + """Write a descriptions cache of *name*`=`*cached_version* entries. + + Each entry records the package the server is configured with, so the + identity gate is satisfied for `pkg` and the version comparison alone + decides staleness. `local` records the `"{command} {args}"` fallback + that `refresh_server` writes for an unclassifiable server, with the + `"unknown"` type that makes its identity permanently unconfirmable. + """ + packages = { + "pkg": ("some-pkg", "npm"), + "local": ("node /opt/srv.js", "unknown"), + } + cache_dir.mkdir(parents=True, exist_ok=True) + (cache_dir / "descriptions.yaml").write_text( + yaml.safe_dump( + { + "generated_at": "2025-01-01T00:00:00Z", + "gateway_version": "1.0.0", + "servers": { + name: { + "package": packages[name][0], + "package_type": packages[name][1], + "version": version, + "generated_at": "2025-01-01T00:00:00Z", + "capability_summary": f"{name} summary", + "tools": [], + } + for name, version in entries.items() + }, + } + ) + ) + + @staticmethod + async def _fake_version( + command: str, args: list[str], timeout: float = 10.0 + ) -> tuple[str | None, str]: + """Stand in for the network: npm resolves, anything else does not.""" + if command == "npx": + return ("2.0.0", "npm") + return (None, "unknown") + + async def _run(self, cache_dir: Path, *names: str) -> None: + with patch( + "sys.argv", + [ + "pmcp", + "refresh", + "--check-versions", + "--cache-dir", + str(cache_dir), + ], + ): + args = parse_args() + + with patch( + "pmcp.manifest.refresher.load_manifest", + return_value=self._manifest(*names), + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new=AsyncMock(side_effect=self._fake_version), + ): + await async_main(args) + + @pytest.mark.asyncio + async def test_unverifiable_only_is_not_offered_the_force_remedy( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The permanent-false-alarm case: report it, but do not prescribe. + + `pmcp refresh --force` cannot settle this entry -- the next check still + cannot confirm the package -- so printing that footer sends an operator + after a fix that will not hold. + """ + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "cache" + self._write_cache(cache_dir, local="1.0.0") + + await self._run(cache_dir, "local") + + out = capsys.readouterr().out + assert self.UNVERIFIABLE_HEADING in out, ( + f"an unconfirmable server was not reported separately: {out!r}" + ) + assert "local" in out + assert self.FORCE_FOOTER not in out, ( + "an unconfirmable server was offered --force, which cannot settle " + f"it: {out!r}" + ) + assert self.NEWER_HEADING not in out, ( + f"an unconfirmable server was reported as out of date: {out!r}" + ) + + @pytest.mark.asyncio + async def test_genuinely_stale_still_prints_the_old_report( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A real newer version is actionable and keeps its remedy verbatim.""" + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "cache" + self._write_cache(cache_dir, pkg="1.0.0") + + await self._run(cache_dir, "pkg") + + out = capsys.readouterr().out + assert "Found 1 servers with newer versions:" in out, out + assert "pkg: 1.0.0 -> 2.0.0" in out, out + assert "Run 'pmcp refresh --force' to update." in out, out + assert self.UNVERIFIABLE_HEADING not in out, ( + f"a genuinely stale server was reported as unconfirmable: {out!r}" + ) + + @pytest.mark.asyncio + async def test_mixed_result_sorts_each_server_into_its_own_group( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """Membership, not mere presence -- both names print either way. + + The footer is asserted to appear once and to fall in the stale section, + because a footer trailing the whole report reads as a remedy for the + unconfirmable servers too. + """ + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "cache" + self._write_cache(cache_dir, pkg="1.0.0", local="1.0.0") + + await self._run(cache_dir, "pkg", "local") + + out = capsys.readouterr().out + assert self.UNVERIFIABLE_HEADING in out, out + stale_section, _, unverifiable_section = out.partition( + self.UNVERIFIABLE_HEADING + ) + assert "pkg: 1.0.0 -> 2.0.0" in stale_section, ( + f"the genuinely stale server was not in the stale group: {out!r}" + ) + assert "local" in unverifiable_section, ( + f"the unconfirmable server was not in its own group: {out!r}" + ) + assert "local" not in stale_section, ( + f"the unconfirmable server was also reported as stale: {out!r}" + ) + assert out.count(self.FORCE_FOOTER) == 1, out + assert self.FORCE_FOOTER in stale_section, ( + "the --force footer trails the unconfirmable group, so it reads as " + f"a remedy for it: {out!r}" + ) + + @pytest.mark.asyncio + async def test_nothing_to_report_is_unchanged( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The all-clear line is what operators grep for; leave it alone.""" + monkeypatch.chdir(tmp_path) + cache_dir = tmp_path / "cache" + self._write_cache(cache_dir, pkg="2.0.0") + + await self._run(cache_dir, "pkg") + + out = capsys.readouterr().out + assert "All cached descriptions are up to date." in out, out + assert self.UNVERIFIABLE_HEADING not in out, out + assert self.FORCE_FOOTER not in out, out diff --git a/tests/test_refresher.py b/tests/test_refresher.py index ddeab2d2..0a388573 100644 --- a/tests/test_refresher.py +++ b/tests/test_refresher.py @@ -482,6 +482,11 @@ async def test_stale_server_detected(self, mock_manifest: MagicMock) -> None: servers={ "server1": GeneratedServerDescriptions( package="@test/mcp", + # Matches `npx -y @test/mcp` in `mock_manifest`. Without a + # recorded type the identity gate cannot confirm the entry + # and reports it stale, which is a different code path from + # the version comparison these tests exist to pin. + package_type="npm", version="1.0.0", generated_at="2025-01-01T00:00:00Z", capability_summary="Test", @@ -518,6 +523,11 @@ async def test_up_to_date_server_not_flagged( servers={ "server1": GeneratedServerDescriptions( package="@test/mcp", + # Matches `npx -y @test/mcp` in `mock_manifest`. Without a + # recorded type the identity gate cannot confirm the entry + # and reports it stale, which is a different code path from + # the version comparison these tests exist to pin. + package_type="npm", version="1.0.0", generated_at="2025-01-01T00:00:00Z", capability_summary="Test", @@ -551,6 +561,11 @@ async def test_version_lookup_failure(self, mock_manifest: MagicMock) -> None: servers={ "server1": GeneratedServerDescriptions( package="@test/mcp", + # Matches `npx -y @test/mcp` in `mock_manifest`. Without a + # recorded type the identity gate cannot confirm the entry + # and reports it stale, which is a different code path from + # the version comparison these tests exist to pin. + package_type="npm", version="1.0.0", generated_at="2025-01-01T00:00:00Z", capability_summary="Test", @@ -602,8 +617,17 @@ async def test_unorderable_fetched_version_does_not_short_circuit( self, temp_dir: Path ) -> None: """A cached release vs an unorderable fetched version must refresh.""" + # `package_type` is load-bearing, not decoration. Without it the + # identity gate refuses on an unknown type and the short-circuit's + # `and` chain never reaches `compare_versions` -- this test would still + # pass, but for the identity reason, pinning nothing about the version + # comparison it exists to guard. Verified by mutation: with the type + # omitted, rewriting the short-circuit to `!= "newer"` (so an + # incomparable pair reads as up to date -- the exact + # Consiliency/pmcp#156/#164 fail-open) leaves the whole suite green. existing = GeneratedServerDescriptions( package="srv", + package_type="npm", version="1.0.0", generated_at="2025-01-01T00:00:00Z", capability_summary="stale summary", @@ -647,21 +671,30 @@ async def fake_version(command, args, timeout=None): async def test_incomparable_pair_does_not_short_circuit( self, temp_dir: Path ) -> None: - """A cached npm version vs a fetched digest must refresh, not skip. + """A cached tag vs a fetched digest must refresh, not skip. The case two unary orderability guards let through: `1.0.0` and `abcdef123456` are each orderable, so both guards passed, but the pair is incomparable, the comparator failed closed, and the negation - reported "up to date" -- returning the stale npm cache for what is now - a docker server. `refresh_all` reuses a cache entry by server NAME and - never checks that the package still matches, which is what makes this - reachable. + reported "up to date". + + Both sides are deliberately the SAME package and the SAME ecosystem, so + that `_same_package` CONFIRMS identity and the short-circuit's `and` + chain actually reaches `compare_versions`. This test originally paired + an npm cache against a docker config; the identity gate added in + UPDPATH now refuses that pair on ecosystem alone, which would leave + this test passing for the identity reason and pinning nothing about the + incomparable-version path it exists to guard. That cross-ecosystem case + has its own coverage in `TestPackageIdentityGate`; this one keeps the + version-comparison guard load-bearing. Verified by mutation -- rewriting + the short-circuit to `!= "newer"` fails this test. """ existing = GeneratedServerDescriptions( package="srv", + package_type="docker", version="1.0.0", generated_at="2025-01-01T00:00:00Z", - capability_summary="stale npm summary", + capability_summary="stale summary from a tagged build", tools=[], ) server = ServerConfig( @@ -710,6 +743,11 @@ async def test_short_circuit_is_a_single_compare_versions_call( ) -> None: existing = GeneratedServerDescriptions( package="srv", + # The identity gate added in Consiliency/pmcp#178 runs ahead of the + # comparison, so the pair has to be confirmably the same package + # for the short-circuit to be reached at all. `result is existing` + # below is what catches a degenerate always-False gate. + package_type="npm", version="1.0.0", generated_at="2025-01-01T00:00:00Z", capability_summary="stale summary", @@ -849,6 +887,587 @@ def test_descriptions_cache_creation(self) -> None: assert cache.servers == {} +def _server_config(command: str, args: list[str], name: str = "srv") -> ServerConfig: + """A minimal manifest server entry.""" + return ServerConfig( + name=name, + description="", + keywords=[], + install={}, + command=command, + args=args, + ) + + +def _cached_entry( + package: str, + package_type: str | None, + version: str = "1.0.0", +) -> GeneratedServerDescriptions: + """A cached description entry for `package`, as the cache would hold it.""" + return GeneratedServerDescriptions( + package=package, + package_type=package_type, + version=version, + generated_at="2025-01-01T00:00:00Z", + capability_summary=f"stale {package or ''} summary", + tools=[], + ) + + +def _write_cache_file( + cache_path: Path, + entries: dict[str, tuple[str, str | None]], +) -> None: + """Write a descriptions cache holding `name -> (package, package_type)`. + + Written as raw YAML rather than through `save_descriptions_cache` so the + fixture does not depend on the writer this lane is also changing. + """ + cache_path.parent.mkdir(parents=True, exist_ok=True) + cache_path.write_text( + yaml.safe_dump( + { + "generated_at": "2025-01-01T00:00:00Z", + "gateway_version": "1.0.0", + "servers": { + name: { + "package": package, + "package_type": package_type, + "version": "1.0.0", + "generated_at": "2025-01-01T00:00:00Z", + "capability_summary": f"stale {package} summary", + "tools": [], + } + for name, (package, package_type) in entries.items() + }, + } + ) + ) + + +def _manifest(servers: dict[str, ServerConfig]) -> Manifest: + return Manifest( + version="1.0", + cli_alternatives={}, + servers=servers, + discovery_queue_path=".mcp-gateway/discovery_queue.json", + ) + + +async def _equal_version(command: str, args: list[str], timeout: float | None = None): + """Every package resolves to the same npm version -- so only identity can + distinguish the cached entry from the configured one.""" + return ("1.0.0", "npm") + + +class TestPackageIdentityGate: + """A cached entry is paired with a server config by NAME, and freshness is + then decided by comparing VERSIONS only. Nothing asks whether the cache + still describes the same PACKAGE (Consiliency/pmcp#178, EC-UPDPATH-1..3). + + Two swaps slip past a versions-only check at an equal version: + + 1. **Same ecosystem** -- `old-pkg@1.0.0` cached, `new-pkg@1.0.0` + configured, both npm, both orderable, equal. + 2. **Cross ecosystem** -- `GeneratedServerDescriptions.package` is a bare + name with no ecosystem, so pypi `foo@1.0.0` reads as the same package as + npm `foo@1.0.0`, and npm/pypi/cargo all produce orderable *release* + versions so `incomparable` never fires. + + The docker case needs no test here: a version against a digest classifies + as `compare_versions(...) == "incomparable"`, which is not `"not_newer"`, + so the short-circuit already does not fire (`TestUpToDateShortCircuit`). + + `refresh_server` returning `None` is the discriminator throughout: a + short-circuit returns the cached object itself, while a real refresh + reaches `stdio_client` -- patched here to raise -- and is caught into + `None`. Asserting on the return value alone cannot otherwise tell + "refreshed" from "returned the stale cache", since both yield a cache + object. + """ + + @pytest.mark.asyncio + async def test_same_ecosystem_swap_refreshes_via_refresh_server(self) -> None: + existing = _cached_entry("old-pkg", "npm") + server = _server_config("npx", ["-y", "new-pkg"]) + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=_equal_version + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("stop after the identity decision"), + ): + result = await refresh_server(server, existing_cache=existing) + + assert result is not existing, ( + "short-circuited as up to date across an npm package swap at an " + "equal version -- old-pkg's descriptions would be served for " + "new-pkg forever" + ) + assert result is None + + @pytest.mark.asyncio + async def test_cross_ecosystem_swap_refreshes_via_refresh_server(self) -> None: + """Same bare name, different ecosystem, equal orderable versions.""" + existing = _cached_entry("foo", "pypi") + server = _server_config("npx", ["-y", "foo"]) + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=_equal_version + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("stop after the identity decision"), + ): + result = await refresh_server(server, existing_cache=existing) + + assert result is not existing, ( + "short-circuited across a pypi -> npm swap of the same bare name: " + "a name alone cannot express package identity" + ) + assert result is None + + @pytest.mark.asyncio + async def test_same_ecosystem_swap_is_reported_stale_by_check_staleness( + self, + ) -> None: + """`pmcp refresh --check-versions` must not print "up to date" here.""" + cache = DescriptionsCache( + generated_at="2025-01-01T00:00:00Z", + gateway_version="1.0.0", + servers={"srv": _cached_entry("old-pkg", "npm")}, + ) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + with patch( + "pmcp.manifest.refresher.load_descriptions_cache", return_value=cache + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new_callable=AsyncMock, + return_value=("1.0.0", "npm"), + ): + result = await check_staleness(manifest=manifest) + + assert "srv" in result, ( + "a cache for old-pkg against a config for new-pkg reported as up " + "to date -- operator-visible via `pmcp refresh --check-versions`" + ) + assert result["srv"] == ("1.0.0", "1.0.0") + + @pytest.mark.asyncio + async def test_cross_ecosystem_swap_is_reported_stale_by_check_staleness( + self, + ) -> None: + cache = DescriptionsCache( + generated_at="2025-01-01T00:00:00Z", + gateway_version="1.0.0", + servers={"srv": _cached_entry("foo", "pypi")}, + ) + manifest = _manifest({"srv": _server_config("npx", ["-y", "foo"])}) + + with patch( + "pmcp.manifest.refresher.load_descriptions_cache", return_value=cache + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new_callable=AsyncMock, + return_value=("1.0.0", "npm"), + ): + result = await check_staleness(manifest=manifest) + + assert "srv" in result, ( + "a pypi cache against an npm config of the same bare name " + "reported as up to date" + ) + + @pytest.mark.asyncio + async def test_swap_does_not_survive_refresh_all(self) -> None: + """EC-3: `refresh_all` assembles the (cached, configured) pair itself, + so it must not be able to bypass the gate.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file(cache_path, {"srv": ("old-pkg", "npm")}) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + with patch( + "pmcp.manifest.refresher.get_package_version", + side_effect=_equal_version, + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("no server to connect to"), + ): + cache = await refresh_all(manifest=manifest, cache_path=cache_path) + + returned = cache.servers.get("srv") + assert returned is None or returned.package != "old-pkg", ( + "refresh_all returned old-pkg's descriptions for a server now " + "configured to run new-pkg" + ) + + reloaded = load_descriptions_cache(cache_path) + assert reloaded is not None + saved = reloaded.servers.get("srv") + assert saved is None or saved.package != "old-pkg", ( + "refresh_all wrote old-pkg's descriptions back to disk" + ) + + @pytest.mark.asyncio + async def test_cross_ecosystem_swap_does_not_survive_refresh_all(self) -> None: + """The cross-ecosystem swap at the third site: same bare name, so only + the recorded type distinguishes the cached entry from the config.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file(cache_path, {"srv": ("foo", "pypi")}) + manifest = _manifest({"srv": _server_config("npx", ["-y", "foo"])}) + + with patch( + "pmcp.manifest.refresher.get_package_version", + side_effect=_equal_version, + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("no server to connect to"), + ): + cache = await refresh_all(manifest=manifest, cache_path=cache_path) + + # The bare name is identical on both sides, so asserting on + # `package` cannot tell the entries apart -- the pypi entry either + # survived regeneration or it did not. + assert "srv" not in cache.servers, ( + "refresh_all kept the pypi cache for a server now configured " + "to run the npm package of the same name" + ) + + reloaded = load_descriptions_cache(cache_path) + assert reloaded is None or "srv" not in reloaded.servers, ( + "refresh_all wrote the pypi descriptions back to disk" + ) + + def test_package_type_survives_a_cache_round_trip(self) -> None: + """IF-0-UPDPATH-2: populated where the entry is WRITTEN, not only where + it is read -- otherwise every refreshed entry reloads as unknown and + the gate refreshes forever.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + save_descriptions_cache( + DescriptionsCache( + generated_at="2025-01-01T00:00:00Z", + gateway_version=GATEWAY_VERSION, + servers={"srv": _cached_entry("new-pkg", "npm")}, + ), + cache_path, + ) + + reloaded = load_descriptions_cache(cache_path) + assert reloaded is not None + assert reloaded.servers["srv"].package_type == "npm" + + def test_absent_package_type_reads_as_none_not_empty_string(self) -> None: + """An absent value must stay distinguishable from a recorded empty one + (IF-0-UPDPATH-2), so pre-phase caches read as unknown.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + cache_path.write_text( + yaml.safe_dump( + { + "generated_at": "2025-01-01T00:00:00Z", + "gateway_version": "1.0.0", + "servers": { + "srv": { + "package": "new-pkg", + "version": "1.0.0", + "generated_at": "2025-01-01T00:00:00Z", + "capability_summary": "legacy entry", + "tools": [], + } + }, + } + ) + ) + + reloaded = load_descriptions_cache(cache_path) + assert reloaded is not None + assert reloaded.servers["srv"].package_type is None + + @pytest.mark.asyncio + async def test_regenerated_entry_records_its_package_type(self) -> None: + """A freshly generated entry carries the type it was generated for.""" + server = _server_config("npx", ["-y", "new-pkg"]) + + class _FakeStdio: + async def __aenter__(self): + return (None, None) + + async def __aexit__(self, *exc): + return False + + tools_result = MagicMock() + tools_result.tools = [] + fake_session = MagicMock() + fake_session.initialize = AsyncMock() + fake_session.list_tools = AsyncMock(return_value=tools_result) + + class _FakeSession: + def __init__(self, *args, **kwargs) -> None: + pass + + async def __aenter__(self): + return fake_session + + async def __aexit__(self, *exc): + return False + + async def fake_version(command, args, timeout=None): + return ("2.0.0", "npm") + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=fake_version + ): + with patch("mcp.client.stdio.stdio_client", return_value=_FakeStdio()): + with patch("mcp.ClientSession", _FakeSession): + result = await refresh_server(server) + + assert result is not None + assert result.package == "new-pkg" + assert result.package_type == "npm" + + +class TestUnknownPackageForcesRefresh: + """EC-UPDPATH-7. The gate must read an unknown side as "cannot confirm + identity -> refresh", NEVER as "cannot compare -> skip the check". The + second phrasing is the natural one to reach for, passes a naive suite, and + is the same fail-open collapse as `not is_version_newer(...)`, which + shipped three times (Consiliency/pmcp#155, #156, #163) before the tri-state + migration deleted the wrappers to make it unrepresentable. + + `test_absent_cached_package_type_forces_refresh` is the discriminating one: + the package NAMES match and the VERSIONS match, so only the unknown type + can force the refresh. Under "only compare when both sides are known" it + short-circuits and the test fails. + + The configured-unknown arms have no possible behavioural test -- + `refresh_server` falls back to `pkg_name = f"{command} {args}"`, so the + configured name is never empty -- which is why the predicate is asserted + directly below. + """ + + @pytest.mark.asyncio + async def test_absent_cached_package_type_forces_refresh(self) -> None: + existing = _cached_entry("new-pkg", None) + server = _server_config("npx", ["-y", "new-pkg"]) + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=_equal_version + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("stop after the identity decision"), + ): + result = await refresh_server(server, existing_cache=existing) + + assert result is not existing, ( + "an unrecorded package_type was read as 'cannot compare, skip the " + "check' rather than 'cannot confirm identity, refresh'" + ) + assert result is None + + @pytest.mark.asyncio + async def test_unknown_cached_package_type_forces_refresh(self) -> None: + existing = _cached_entry("new-pkg", "unknown") + server = _server_config("npx", ["-y", "new-pkg"]) + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=_equal_version + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("stop after the identity decision"), + ): + result = await refresh_server(server, existing_cache=existing) + + assert result is not existing + assert result is None + + @pytest.mark.asyncio + async def test_empty_cached_package_forces_refresh(self) -> None: + existing = _cached_entry("", "npm") + server = _server_config("npx", ["-y", "new-pkg"]) + + with patch( + "pmcp.manifest.refresher.get_package_version", side_effect=_equal_version + ): + with patch( + "mcp.client.stdio.stdio_client", + side_effect=RuntimeError("stop after the identity decision"), + ): + result = await refresh_server(server, existing_cache=existing) + + assert result is not existing + assert result is None + + @pytest.mark.asyncio + async def test_absent_cached_package_type_is_stale_via_check_staleness( + self, + ) -> None: + """Every cache written before this phase reads as unknown, so the + migration forces one refresh rather than silently trusting the entry.""" + cache = DescriptionsCache( + generated_at="2025-01-01T00:00:00Z", + gateway_version="1.0.0", + servers={"srv": _cached_entry("new-pkg", None)}, + ) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + with patch( + "pmcp.manifest.refresher.load_descriptions_cache", return_value=cache + ): + with patch( + "pmcp.manifest.refresher.get_package_version", + new_callable=AsyncMock, + return_value=("1.0.0", "npm"), + ): + result = await check_staleness(manifest=manifest) + + assert "srv" in result + + def test_predicate_matches_a_fully_known_identical_pair(self) -> None: + from pmcp.manifest.refresher import _same_package + + assert _same_package("x", "npm", "x", "npm") is True + + def test_predicate_refuses_every_unknown_arm(self) -> None: + from pmcp.manifest.refresher import _same_package + + # cached name unknown + assert _same_package("", None, "x", "npm") is False + assert _same_package("", "npm", "x", "npm") is False + assert _same_package("unknown", "npm", "unknown", "npm") is False + # cached type unknown, name known on both sides + assert _same_package("x", None, "x", "npm") is False + assert _same_package("x", "", "x", "npm") is False + assert _same_package("x", "unknown", "x", "unknown") is False + # configured name unknown + assert _same_package("x", "npm", None, "npm") is False + assert _same_package("x", "npm", "", "npm") is False + # configured type unknown + assert _same_package("x", "npm", "x", None) is False + assert _same_package("x", "npm", "x", "") is False + assert _same_package("x", "npm", "x", "unknown") is False + + def test_predicate_refuses_a_differing_pair(self) -> None: + from pmcp.manifest.refresher import _same_package + + assert _same_package("x", "npm", "y", "npm") is False + assert _same_package("foo", "pypi", "foo", "npm") is False + + +class TestMismatchedCacheNeverSurvivesFailure: + """`refresh_all` has two paths that put a cached entry into the saved cache + without regenerating it: the `None`/raise fallback inside `refresh_target`, + and the final merge loop that re-adds every cached entry missing from the + new set. An identity-mismatched entry must survive neither. + + Both tests below require both fixes: closing only the fallback lets the + merge loop write the entry back, and closing only the merge loop leaves the + fallback returning it as a live result. Dropping the entry on a failed + regeneration is acceptable; writing the wrong package's descriptions back + is not. + """ + + @pytest.mark.asyncio + async def test_failed_refresh_does_not_write_mismatched_entry_back(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file(cache_path, {"srv": ("old-pkg", "npm")}) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + async def failing_refresh(server_config, existing_cache=None, force=False): + return None + + with patch( + "pmcp.manifest.refresher.refresh_server", side_effect=failing_refresh + ): + cache = await refresh_all(manifest=manifest, cache_path=cache_path) + + assert "srv" not in cache.servers, ( + "a failed regeneration handed old-pkg's descriptions back for a " + "server now configured to run new-pkg" + ) + reloaded = load_descriptions_cache(cache_path) + assert reloaded is None or "srv" not in reloaded.servers + + @pytest.mark.asyncio + async def test_raising_refresh_does_not_write_mismatched_entry_back(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file(cache_path, {"srv": ("old-pkg", "npm")}) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + async def raising_refresh(server_config, existing_cache=None, force=False): + raise RuntimeError("regeneration blew up") + + with patch( + "pmcp.manifest.refresher.refresh_server", side_effect=raising_refresh + ): + cache = await refresh_all(manifest=manifest, cache_path=cache_path) + + assert "srv" not in cache.servers + reloaded = load_descriptions_cache(cache_path) + assert reloaded is None or "srv" not in reloaded.servers + + @pytest.mark.asyncio + async def test_matching_entry_still_survives_a_failed_refresh(self) -> None: + """The gate must drop mismatched entries only -- a degenerate + always-drop would pass the two tests above for the wrong reason.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file(cache_path, {"srv": ("new-pkg", "npm")}) + manifest = _manifest({"srv": _server_config("npx", ["-y", "new-pkg"])}) + + async def failing_refresh(server_config, existing_cache=None, force=False): + return None + + with patch( + "pmcp.manifest.refresher.refresh_server", side_effect=failing_refresh + ): + cache = await refresh_all(manifest=manifest, cache_path=cache_path) + + assert cache.servers["srv"].package == "new-pkg" + + @pytest.mark.asyncio + async def test_untargeted_entry_still_survives_the_merge(self) -> None: + """The merge loop's purpose -- keeping servers outside the target list + -- must be preserved.""" + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "descriptions.yaml" + _write_cache_file( + cache_path, + {"srv": ("new-pkg", "npm"), "other": ("other-pkg", "npm")}, + ) + manifest = _manifest( + { + "srv": _server_config("npx", ["-y", "new-pkg"]), + "other": _server_config("npx", ["-y", "other-pkg"], name="other"), + } + ) + + async def failing_refresh(server_config, existing_cache=None, force=False): + return None + + with patch( + "pmcp.manifest.refresher.refresh_server", side_effect=failing_refresh + ): + cache = await refresh_all( + manifest=manifest, cache_path=cache_path, servers=["srv"] + ) + + assert cache.servers["other"].package == "other-pkg" + + # Pytest fixture from conftest @pytest.fixture def temp_dir() -> Path: diff --git a/tests/test_tools.py b/tests/test_tools.py index 8a6a0eeb..88bae3f5 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -5206,6 +5206,85 @@ async def test_update_server_bumps_descriptions_cache_version( assert result.latest_version == "0.2.0" assert cache.servers["playwright"].version == "0.2.0" + @pytest.mark.asyncio + async def test_update_server_drops_an_entry_whose_package_changed( + self, monkeypatch: pytest.MonkeyPatch + ): + """A package CHANGE invalidates the entry; it is not patched in place. + + `update_server` is a third write site for package identity, alongside + `refresh_server`'s constructor and `save_descriptions_cache`'s dict. + Patching `version`/`package`/`tools` field-by-field across a package + change leaves a permanently MIXED entry: `capability_summary` is + deliberately preserved on a version bump, but once `package` and + `version` both name the new package the freshness short-circuit + CONFIRMS identity and never regenerates it -- so the old package's + summary is served under the new package's label forever, not one + refresh cycle behind. Dropping the entry is this phase's own rule + applied here: cannot confirm the cache describes this package -> + refresh (ah board review, red-team seat). + + The entry must start labelled with a DIFFERENT package or this test + sees nothing: `_make_cache` uses `@playwright/mcp`, exactly what + `detect_package_type` returns here, so any assertion about that value + holds whether or not the code under test ran at all. An earlier version + of this test made precisely that mistake -- deleting the package + assignment left it green -- which is the hollow-pin failure mode this + change exists to close, found independently by two board seats. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + cache.servers["playwright"].package = "@old-vendor/mcp" + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + assert "playwright" not in cache.servers, ( + "an entry describing a different package must be dropped for " + "wholesale regeneration, not patched into a mixed entry carrying " + "the old package's capability_summary under the new package's name" + ) + + @pytest.mark.asyncio + async def test_update_server_backfills_package_type_on_a_same_package_bump( + self, monkeypatch: pytest.MonkeyPatch + ): + """A same-package version bump keeps the entry and records its type. + + A cache written before the entry carried a package type reads as + unknown to `_same_package`, which forces a refresh every time. Since + `update_server` has just classified the package, this is the cheapest + place to backfill it -- and without it such an entry stays permanently + unverifiable. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + # Same package as configured, but legacy: no recorded type. + assert cache.servers["playwright"].package == "@playwright/mcp" + assert cache.servers["playwright"].package_type is None + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + entry = cache.servers["playwright"] + assert entry.version == "0.2.0" + assert entry.package_type == "npm", ( + "a legacy entry stays permanently unverifiable if update_server " + "does not record the package type it just detected" + ) + @pytest.mark.asyncio async def test_update_server_persists_version_across_restart( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -5428,6 +5507,17 @@ async def test_update_server_uses_configured_override_not_manifest( "pmcp.tools.handlers.load_configs", lambda **_: [configured_override] ) cache = self._make_cache(version="0.1.0") + # Label the entry with the OVERRIDE's package, which is what is + # actually configured here. `_make_cache` uses the manifest's + # `@playwright/mcp`, and against the `different-playwright-fork` + # override that is a genuine package change -- which `update_server` + # now correctly responds to by DROPPING the entry for wholesale + # regeneration, leaving nothing for this version assertion to read. + # This test is about override resolution driving the probe, not about + # cache-identity handling (which + # `test_update_server_drops_an_entry_whose_package_changed` owns), so + # matching the label keeps it isolated to its own subject. + cache.servers["playwright"].package = "different-playwright-fork" client_manager = MockClientManager() gt = GatewayTools( client_manager=client_manager, # type: ignore @@ -6302,3 +6392,327 @@ def test_feedback_events_are_sanitized_before_preview(self) -> None: assert "user:pass" not in json.dumps(tools._feedback_events) assert "secret-jwt" not in json.dumps(tools._feedback_events) + + +class TestUpdateServerAmbientEnvironment: + """What actually happens to the child environment across `update_server`'s + probe window -- two different contracts that an earlier single-sentence + reading of this area conflated (phase UPDPATH, EC-4). + + `update_server` probes for a new package version, then re-resolves the + config and refuses to restart if it changed (the Consiliency/pmcp#151 + TOCTOU guard). Which side of that guard an environment change lands on + depends entirely on whether the variable is CONFIG-DRIVEN or AMBIENT: + + (a) A manifest credential is config-driven, because `_manifest_server_to_config` + (`config/loader.py:1025`) resolves it INTO `LocalMcpServerConfig.env` at + load time. Rotate it inside the probe window and the recheck's resolved + `env` differs from the probe's, so the guard refuses -- the package is + fetched but NOT activated. That is the guarantee working as designed + ("the config restarted onto is the config that was probed"), not a + defect; the rotation applies on the next update. + + (b) A genuinely ambient variable -- one no config declares as an explicit + `env` override -- is LIVE at spawn. Both sides of the guard derive from + the single `stripped_base` read in `update_server`, so an ambient change + cancels out and can never cause a refusal, and `sanitized_subprocess_env` + (`env_store.py:138`) calls `os.environ.copy()` at spawn time -- the same + read connect, refresh and auto-reconnect reach. + + Test (b) is written against the environment actually handed to + `asyncio.create_subprocess_exec`, with a REAL `ClientManager._connect_stdio` + building it. Two shapes of this test pass vacuously and are deliberately + avoided: a fully mocked ClientManager has no spawn to capture at all (the + env is built inside `_connect_stdio`, `client/manager.py:2151`), and + rotating a PMCP-MANAGED key would exercise `sanitized_subprocess_env`'s + `own` arm -- which is (a)'s path, not (b)'s -- because the strip removes + every managed key from the base and re-applies only the server's own + resolved value. Asserting on `os.environ` itself would pass without the + contract holding. + """ + + _AMBIENT_KEY = "SL2_AMBIENT_VAR" + _CREDENTIAL_KEY = "SL2_MANIFEST_TOKEN" + + def _install_manifest( + self, + monkeypatch: pytest.MonkeyPatch, + *, + env_var: str | None, + ) -> Manifest: + """Install a one-server manifest as the only resolution source. + + `load_configs` returns nothing so `_resolve_lifecycle_config` falls + through to the manifest instead of the real filesystem, exactly as the + neighbouring update_server suites do. + """ + manifest = Manifest( + version="1.0", + cli_alternatives={}, + servers={ + "ambient": ServerConfig( + name="ambient", + description="Fixture server", + keywords=["fixture"], + install={}, + command="npx", + args=["-y", "@example/mcp"], + requires_api_key=env_var is not None, + env_var=env_var, + ) + }, + discovery_queue_path=".mcp-gateway/discovery_queue.json", + ) + monkeypatch.setattr("pmcp.tools.handlers.load_manifest", lambda: manifest) + monkeypatch.setattr("pmcp.tools.handlers.load_configs", lambda **_: []) + # Keep the strip hermetic: unpatched, sanitized_subprocess_env reads the + # operator's real ~/.config/pmcp store inside the guard. Empty also makes + # _AMBIENT_KEY unambiguously NON-managed, which is what (b) requires. + monkeypatch.setattr( + "pmcp.env_store.managed_secret_keys", lambda project=None: set() + ) + return manifest + + def _make_cache(self) -> DescriptionsCache: + return DescriptionsCache( + generated_at="2024-01-01T00:00:00", + gateway_version="1.0.0", + servers={ + "ambient": GeneratedServerDescriptions( + package="@example/mcp", + version="0.1.0", + generated_at="2024-01-01T00:00:00", + capability_summary="Fixture server", + tools=[], + ) + }, + ) + + @pytest.mark.asyncio + async def test_manifest_credential_rotated_in_probe_window_is_refused( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """(a) A manifest credential rotating mid-probe REFUSES the restart. + + Counter-intuitive but correct, and the reason this lane documents two + contracts rather than one. The credential reaches the child process + through `LocalMcpServerConfig.env`, which the manifest resolver fills in + at load time -- so it is a CONFIG-driven value, and the recheck sees a + different one than the probe did. The guard fires before refresh() and + before any restart, so nothing is activated and no version is recorded. + """ + manifest = self._install_manifest(monkeypatch, env_var=self._CREDENTIAL_KEY) + monkeypatch.setenv(self._CREDENTIAL_KEY, "OLD-TOKEN") + + cache = self._make_cache() + client_manager = MockClientManager() + gt = GatewayTools( + client_manager=client_manager, # type: ignore[arg-type] + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + refresh_called = False + + async def _tracking_refresh(input_data: dict[str, Any]) -> Any: + nonlocal refresh_called + refresh_called = True + return types.SimpleNamespace(ok=True) + + monkeypatch.setattr(gt, "refresh", _tracking_refresh) + + async def _fake_get_package_version( + command: str, args: list[str], timeout: float = 5.0 + ) -> tuple[str | None, str]: + return ("0.2.0", "npm") + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", _fake_get_package_version + ) + + async def _probe_that_rotates_the_credential( + command: list[str], env: dict[str, str] | None = None + ) -> tuple[bool, str]: + # The probe env is built from the config resolved BEFORE the probe; + # asserting the old value here is what proves the rotation below + # lands strictly inside the probe window rather than before it. + assert env is not None + assert env.get(self._CREDENTIAL_KEY) == "OLD-TOKEN", ( + f"probe env should carry the pre-rotation credential, " + f"got {env.get(self._CREDENTIAL_KEY)!r}" + ) + monkeypatch.setenv(self._CREDENTIAL_KEY, "ROTATED-TOKEN") + return (True, "ok") + + monkeypatch.setattr( + gt, "_run_update_probe_command", _probe_that_rotates_the_credential + ) + + # The load-bearing fact behind this whole contract: the manifest + # resolver puts the credential INTO the config, so a rotation is a + # config difference to the guard, not an ambient one. + pre_probe = manifest_server_to_config(manifest.get_server("ambient")) + assert isinstance(pre_probe.config, LocalMcpServerConfig) + assert pre_probe.config.env == {self._CREDENTIAL_KEY: "OLD-TOKEN"} + + result = await gt.update_server({"server_name": "ambient"}) + + assert result.ok is False + assert "changed while the update was being fetched" in result.message + # Nothing was activated -- not by the restart, and not by refresh(), + # which is itself a diff-based reconcile capable of activating. + assert refresh_called is False + assert not any("disconnect_server" in e for e in client_manager.events) + assert not any("connect_server" in e for e in client_manager.events) + assert cache.servers["ambient"].version == "0.1.0" + + @pytest.mark.asyncio + async def test_ambient_variable_in_probe_window_is_live_at_spawn( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """(b) A genuinely ambient variable does NOT refuse and IS live at spawn. + + The server declares no `env` override at all, so the mutated variable + reaches the child only through `sanitized_subprocess_env`'s + `os.environ.copy()`. The assertion is against the env dict handed to + `asyncio.create_subprocess_exec` by a real `_connect_stdio`; only the + post-spawn JSON-RPC handshake is stubbed, which is downstream of the + env construction under test. + """ + from pmcp.client.manager import ClientManager + + manifest = self._install_manifest(monkeypatch, env_var=None) + # Pins "genuinely ambient": nothing in the resolved config declares this + # key (or any key), so a captured value can only have come from the + # spawn-time os.environ read. + resolved = manifest_server_to_config(manifest.get_server("ambient")) + assert isinstance(resolved.config, LocalMcpServerConfig) + assert resolved.config.env is None + + monkeypatch.setenv(self._AMBIENT_KEY, "before-probe") + + spawned_envs: list[dict[str, str]] = [] + real_manager = ClientManager() + + async def _fake_spawn(*args: Any, **kwargs: Any) -> Any: + spawned_envs.append(dict(kwargs["env"])) + return _FakeStdioProcess() + + monkeypatch.setattr("asyncio.create_subprocess_exec", _fake_spawn) + + # Everything below the spawn boundary: the handshake and the stdout + # reader would need a live MCP peer. `_connect_stdio` itself -- which + # calls sanitized_subprocess_env and passes env= to the spawn -- is + # untouched, and that is the code this test exists to pin. + async def _no_initialize(managed: Any) -> None: + return None + + async def _no_capabilities(managed: Any) -> tuple[int, int, int]: + return (0, 0, 0) + + async def _no_stdout(name: str, managed: Any) -> None: + return None + + monkeypatch.setattr(real_manager, "_send_initialize", _no_initialize) + monkeypatch.setattr(real_manager, "_index_capabilities", _no_capabilities) + monkeypatch.setattr(real_manager, "_read_stdout", _no_stdout) + + cache = self._make_cache() + gt = GatewayTools( + client_manager=real_manager, + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + async def _stub_refresh(input_data: dict[str, Any]) -> Any: + return types.SimpleNamespace(ok=True) + + monkeypatch.setattr(gt, "refresh", _stub_refresh) + + async def _fake_get_package_version( + command: str, args: list[str], timeout: float = 5.0 + ) -> tuple[str | None, str]: + return ("0.2.0", "npm") + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", _fake_get_package_version + ) + + async def _probe_that_mutates_the_ambient_var( + command: list[str], env: dict[str, str] | None = None + ) -> tuple[bool, str]: + assert env is not None + assert env.get(self._AMBIENT_KEY) == "before-probe" + monkeypatch.setenv(self._AMBIENT_KEY, "after-probe") + return (True, "ok") + + monkeypatch.setattr( + gt, "_run_update_probe_command", _probe_that_mutates_the_ambient_var + ) + + result = await gt.update_server({"server_name": "ambient"}) + + # No refusal: the ambient change cancels out across the one + # `stripped_base` both sides of the guard are derived from. + assert "changed while the update was being fetched" not in result.message + assert result.ok is True + assert result.restarted is True + # And it is live at the spawn -- the restarted server received the + # POST-probe value, not the one in force when the update began. + assert len(spawned_envs) == 1, f"expected exactly one spawn, got {spawned_envs}" + assert spawned_envs[0][self._AMBIENT_KEY] == "after-probe" + + +class _FakeStdioProcess: + """A spawned-process stand-in for tests that patch + `asyncio.create_subprocess_exec` at the spawn boundary. + + Only what `_connect_stdio` touches before/after the handshake it stubs: + `stdin` (never written to once `_send_initialize` is stubbed), `stdout`, + and `stderr=None` so no stderr reader task is started. It is deliberately + never disconnected by these tests -- teardown would reach + `_terminate_process_tree`, which signals a real process group. + """ + + def __init__(self) -> None: + self.stdin = None + self.stdout = None + self.stderr = None + self.pid = -1 + self.returncode: int | None = None + + +def test_update_server_docstring_states_both_probe_window_env_contracts() -> None: + """`update_server`'s own docstring must state BOTH halves of EC-4. + + The behaviour is unchanged by this phase; what was wrong was the + description. A single "the environment is live across the update" sentence + is false for every manifest credential, because the manifest resolver puts + it in `LocalMcpServerConfig.env` and the TOCTOU guard then refuses. The + docstring is the only place a reader of this function meets that + distinction, so it is asserted rather than left to review. + """ + doc = GatewayTools.update_server.__doc__ or "" + # Flattened so a phrase assertion pins the wording, not the line wrapping. + lowered = " ".join(doc.lower().split()) + + # Half one: a config-driven change -- explicitly including a manifest + # credential -- is refused rather than restarted onto. + assert "refus" in lowered, "docstring does not state the config-driven refusal" + assert "manifest credential" in lowered, ( + "docstring does not name the manifest credential as config-driven, " + "which is the case an earlier reading got wrong" + ) + assert "the config restarted onto is the config that was probed" in lowered, ( + "docstring does not state the guarantee the refusal exists to uphold" + ) + + # Half two: a genuinely ambient variable is read at spawn time and is live. + assert "ambient" in lowered, "docstring does not state the ambient half" + assert "os.environ" in lowered and "spawn" in lowered, ( + "docstring does not say the ambient value is read from os.environ at spawn time" + ) + assert "cancel" in lowered, ( + "docstring does not explain WHY an ambient change cannot refuse " + "(both guard sides derive from one stripped_base, so it cancels out)" + )