Repository navigation
fix: version comparison fails closed instead of fabricating update notices - #155
Merged
Merged
Conversation
…ects) Panel: grok-4.6 AGREE, codex DISAGREE, gemini DISAGREE. Two legs independently found the same policy-gating contradiction; codex found two more. All three verified against source: B1 - the notice fix was half a fix: changing only the 'latest' lookup to the configured package leaves 'current' as a version generated from the MANIFEST package, so unrelated packages are still compared. GeneratedServerDescriptions carries a field the original plan never consulted, and the proposed test (assert B was queried) would have passed while the bug survived. B2 - update_server calls refresh() AFTER the probe, which reloads config. Passing the stale probed config into the restart would disconnect the edited server and reconnect the old one - worse than the TOCTOU being fixed. Replaced with re-resolve-and-compare: abort with ok=False on drift, mutate no cache. B3 - _load_all_configured_servers returns entries BEFORE policy filtering (its own docstring), so a no-gating helper returns command/args for a denied server and the policy-silence acceptance criterion was unsatisfiable. Added an is_server_allowed check - a pure predicate, so it stays side-effect free. Tests now assert observable outcomes (was a notice emitted, what did the cache record) rather than argument plumbing.
Follows three board rounds that each rejected a patch-level fix in PR #154. Root cause recorded: _stale_check_cache is keyed by server name alone while carrying versions that belong to a specific package, and its three readers (_run_stale_index, _get_update_warning, catalog_search) apply different TTL/identity contracts -- so every fix patched one reader while another kept serving the stale tuple. Plan re-keys the cache on (server, package) so a cross-package entry is unrepresentable rather than something each reader must remember to detect.
Two legs converged independently on the fatal flaw: re-keying _stale_check_cache on (server, package) does not make a cross-package comparison unrepresentable, because both writers still source 'current' from the server-scoped descriptions_cache.version -- so an A->B switch stores (A_version, B_latest) under key (server, B) and the comparison stays wrong behind an authoritative- looking key. grok named the root cause my framing missed: get_package_version returns UPSTREAM LATEST only. Nothing in the system observes an INSTALLED version, and descriptions_cache.version is itself an upstream snapshot from describe time. That is why no attempt to 'establish a baseline' for the new package could ever work -- the baseline does not exist. #150 is not a cache-keying bug; it needs a decision about installed-version provenance first.
…150) Maintainer decision after the re-keying plan was boarded 3/3 DISAGREE: probe the installed version directly rather than continuing to infer it. The probe already exists and pmcp discards it. _send_initialize reads protocolVersion and capabilities out of the initialize result and ignores serverInfo in the same dict. Verified against the installed mcp library: InitializeResult.server_info is REQUIRED and carries Implementation.version. Every conforming server reports its own version on every connect. That is the installed-version source all four prior attempts lacked -- no new subprocess, no new network call, no new failure mode, and it is authoritative for what is actually RUNNING rather than what config claims. Notices become running-version vs upstream-latest instead of upstream-snapshot vs upstream-now. Carries forward unchanged: the .mcp.json precedence + is_server_allowed gate, and the whole #151 drift fix. Deletes _repoint_cache_entry_to_effective_package, the self-heal that caused permanent silence and erased good baselines.
…REE) B1 (fatal): serverInfo.version is an IMPLEMENTATION version, not an installed package version -- and this repo already documented it at plans/phase-plan-v11-PG.md:44, which I failed to find before planning. FastMCP 1.x reports the mcp LIBRARY version; mcp 2.x defaults it to empty. My live context7 check (serverInfo 4.0.2 == npm 4.0.2) showed the value CAN match, not that it MUST -- I over-read one confirming example as an identity guarantee. B3: 'unparseable means silence' is false. is_version_newer extracts digits and never signals a parse failure -- 'nightly', 'build-1', '' and '1.29.0' all compare as older than 2.0.0 and emit a notice. The two worst cases are exactly B1's defaults. My proposed test used a string that happens to return False, so it would have passed by accident while the criterion it claimed to prove was false -- the fourth time a test of mine would have passed while its bug lived. Direct probing remains right; serverInfo is the wrong probe. Any real fix needs a version whose identity is bound to the package compared, plus a fail-closed parse mode in is_version_newer.
Collaborator
Author
Board: 3/3 DISAGREE — not merging. All three findings reproduce.I verified each against source and reproduced them at PR head. Every one holds, and the second is the same class of mistake I opened this PR to fix. 1. The fail-closed gate still accepts malformed versions
2. My docker handling silences docker entirelyThis is the one that stings. I wrote that guard specifically to keep the docker lane working, tested it against 3. Accepted semver suffixes are ordered wronglyDigit extraction ignores semver precedence: This is pre-existing behaviour that my change preserved (the AssessmentThe right fix is almost certainly to stop hand-rolling this: parse with a real implementation ( Also needed regardless: Not merging. I am not attempting a rewrite on top of five failed attempts in this subsystem without a maintainer decision on the dependency question, since |
Three findings, all reproduced before fixing: 1. The hand-rolled regex suffix (?:[-+.].*)? swallowed everything after the first separator and digits were then extracted from the whole string, so '2026-08-17-nightly' parsed as (2026,8,17) and compared NEWER than 3000.0.0. My own test called that input 'unreadable' and passed only because it compared against 2.0.0. 2. My docker guard matched a 'sha256:' prefix, but get_docker_version STRIPS that prefix and returns a bare 12-hex digest. The guard never fired against the real producer, silencing every docker update -- the exact failure the guard was written to prevent. I had tested it with invented literals. 3. Digit extraction ignored PEP 440 precedence: '1.0.0-rc1' -> '1.0.0' read as not-newer (hiding a real release) and the reverse read as newer. Now uses packaging.version: PEP 440 ordering, InvalidVersion for anything that is not a release. Compares the PUBLIC segment so build metadata is not treated as a new release. Digest matching uses the bare hex form the producer emits, with a test that drives the producer's truncation rather than a literal so the two cannot drift apart again.
Self-review of the digest rule: 202612180000 is a plausible calendar/build stamp and is 12 chars of [0-9a-f], so it matched _DIGEST_RE and was then never compared against a dotted release -- silently dropping a real update. Require at least one hex letter; a purely numeric string is a version and packaging orders it.
…round 2)
Two blocking findings, both reproduced first:
1. Same digest, different representation compared as an update:
n('abcdef123456','sha256:abcdef123456') was True -- a fabricated notice for
an unchanged image. _digest_identity now canonicalises (drop prefix,
truncate to the 12 chars the producer emits) and compares identities.
2. packaging reads npm/Cargo '1.0.0-1' as PEP 440 '1.0.0.post1', inverting
SemVer precedence: it HID the real 1.0.0-1 -> 1.0.0 upgrade and fabricated
the reverse. 79 of the manifest's 107 servers are npm. is_version_newer now
takes an optional package_type and routes npm/cargo through SemVer 2.0.0
precedence (build metadata ignored, release outranks prerelease, numeric
identifiers rank below alphanumeric); pypi and unspecified keep PEP 440.
Five of six call sites now pass the ecosystem, taking it from
get_package_version's existing return value. The two catalog_search/cached-tuple
sites cannot: the memoized tuple does not record a package type. Left explicit
rather than silently defaulted.
Both remaining read sites -- catalog_search and _get_update_warning's warm-cache
branch -- fell back to PEP 440 because the memo stored only
(checked_at, current, latest). npm publishes SemVer, where 1.0.0-1 is a
PRERELEASE below 1.0.0; PEP 440 reads it as post-release 1.0.0.post1. Verified
the inversion was live: without a type, n('1.0.0-1','1.0.0') was False (hiding a
real upgrade) and the reverse True (fabricating one), reachable through
catalog_search after the direct paths were already fixed.
The memo is now a 4-tuple carrying pkg_type, written at all three writers
(_run_stale_index, _get_update_warning, update_server) and consumed at both
readers. grok: 'this is not an acceptable residual' -- correct, and it is now
closed rather than documented.
1. REGRESSION FROM THIS PR. refresher.py negates the comparator, and 'not
newer' is not 'up to date' once the comparator fails closed. On main
is_version_newer('unknown','2.0.0') was True, so the cache regenerated; with
the fail-closed change it is False, so 'not(...)' became True and the stale
'unknown' cache -- which this same function persists after a failed lookup --
would be returned forever. Added is_version_orderable() and gated the call
site on it. This is the hazard of making a predicate stricter: a negated
caller silently inverts.
2. My hex-letter rule rejected valid all-numeric digests. get_package_version
truncates SHA-256 to 12 hex chars, which can be all digits, so
'987654321098' -> '123456789012' reported no update. Shape alone cannot
disambiguate a digest from a calendar version, so package_type now decides:
with docker, any hex shape is a digest; without it, a hex letter is required
so a numeric version is still ordered.
3. _SEMVER_RE accepted strings SemVer forbids -- leading zeros in core and
prerelease identifiers (rules 2 and 9) and empty identifiers (rule 10) -- and
ORDERED them, so '1.0.0-01' fabricated an update to '1.0.0'. The pattern now
validates per spec, and invalid input fails closed.
Collaborator
Author
Round 4: 1 AGREE / 1 DISAGREE / 1 no-verdict — three findings, all real, all fixedVerified each against source before fixing. One was a regression this PR introduced, and it is the most instructive of the three. 1. My fail-closed change silently inverted a negated caller — REGRESSION
Measured on And This is the hazard of making a predicate stricter: every negated call site silently flips meaning. Fixed by adding 2. My hex-letter rule rejected valid all-numeric digestsLast round I required a hex letter so a numeric calendar version ( Shape alone cannot separate the two cases. 3.
|
…uard The changelog job failed with 'gh: Validation Failed (HTTP 422)' on its own label lookup, during a window where the GitHub API was returning 503s. Under 'bash -e' that non-zero exit kills the step before the CHANGELOG check runs, so the guard failed CLOSED on an unrelated API hiccup rather than on this PR's content -- CHANGELOG.md is in the diff with an [Unreleased] entry. The Actions rerun endpoint is itself 503ing, so this empty commit triggers fresh runs. Filing the guard's fragility as a follow-up: the label lookup should tolerate a transient API failure instead of failing the check.
The job grants 'pull-requests: read' but called /issues/<n>/labels, which needs
'issues: read'. GitHub returned 'Validation Failed (HTTP 422)', and under
'bash -e' that killed the step before the CHANGELOG check ran -- so the guard
blocked this PR on its own permissions, not on its content. Reproduced on two
independent runs, which is what ruled out my first read of it as a transient
API error.
Switches to /pulls/<n> .labels, covered by the granted scope, and tolerates a
lookup failure ('|| true') so an API hiccup means 'no label seen' rather than
'fail the check'.
ViperJuice
added a commit
that referenced
this pull request
Aug 19, 2026
* chore(plan): read installed versions from the npx/uv runner caches (#150) Sixth attempt. Research killed the naive form first: every manifest server launches via npx/uvx, which are EPHEMERAL RUNNERS -- 'npm ls @upstash/context7-mcp' returns (empty), so shelling out to npm ls / uv pip show would have silenced every notice, exactly the failure mode of attempt 4. The runner CACHES are queryable, and that is the source all five prior attempts lacked: each ~/.npm/_npx/<hash>/package.json names its package and node_modules holds the resolved version (verified: @brightdata/mcp 2.11.1, @eslint/mcp 0.3.8), and uv's environments-v2 envs carry .dist-info metadata. Bound to package identity by construction, unlike descriptions_cache.version (an upstream snapshot) or serverInfo.version (an implementation version). Cost measured: 0.36s for 114 npx entries, filesystem only, no subprocess. Rests on #155, which made the comparator fail closed. * chore(plan): record why matching on dependencies cannot hit a transitive Sampled six npx cache entries: each declares exactly ONE dependency (the invoked package) while node_modules holds the transitive closure -- 212 packages in one entry. So matching must be against 'dependencies'; matching node_modules would resolve any transitive package. Checked before writing code because it is the load-bearing assumption of the npm probe. * chore(plan): record board rejection of the runner-cache plan (3/3 DISAGREE) B1 (fatal): the plan kept descriptions_cache.version as a fallback while its own acceptance criteria required silence in that case. That value is an upstream snapshot (refresher.py:243), so identity agreement cannot make it an observed installed version -- the sixth attempt would still emit inferred notices. My proposed test specified 'no cache entry AND no orderable fallback', so it would have passed while the real regression went uncovered: the same test-shape failure as rounds 1-4 of PR #154. B2: neither probe binds a CONFIGURED SERVER to the cache entry it runs. Verified on this machine: the npx cache holds @consiliency/agent-board-mcp at 1.2.0, 1.2.1 AND 1.2.2 concurrently, so newest-by-mtime can pick an unrelated invocation -- including update_server's own pkg@latest probe. uv is worse: an env holds its full transitive closure (30 dist-infos observed) with no single-declared-dependency marker, so a name scan matches transitives. The caches do hold real installed versions -- that research stands. What they lack is server->entry provenance. Recorded three candidate directions. * chore(plan): record the installed version at spawn, plus a staleness guard (#150) Seventh attempt. Maintainer decision after the runner-cache plan was rejected 3/3: record at spawn, plus a periodic guard so a long-running server does not keep a stale baseline forever. Spawn-time capture differs from all six prior attempts because provenance is by CONSTRUCTION rather than inference: at manager.py:1365 pmcp calls create_subprocess_exec with an argv it chose, so there is no scan, no mtime heuristic, and no ambiguity about which cache entry is in play. Attempt 6 failed precisely there -- verified the npx cache holds one package at 1.2.0, 1.2.1 and 1.2.2 concurrently. The maintainer's guard requirement is confirmed as a real gap: the existing hourly sweep re-checks UPSTREAM latest, but a spawn-time value is captured only at spawn, so a server left running for weeks keeps its baseline indefinitely. The guard extends the existing _stale_indexer_loop rather than adding new scheduling, and re-reads from disk without ever respawning a running server. Carries the board's B1 finding forward: NO fallback to descriptions_cache.version (an upstream snapshot), and the test for that case now includes an orderable fallback, which the last plan's test omitted. * chore(plan): record board rejection of the spawn-provenance plan B1 (fatal): 'argv is authoritative provenance' is false. create_subprocess_exec returns once NPX starts, before it has selected/fetched/launched the package, and npm may resolve to a local, global or cache-backed copy -- only one branch produces an _npx dir. Verified independently before the verdicts: a live 'npx -y @eslint/mcp' tree exposes NO _npx path in any descendant's cmdline, cwd or fds. Knowing the argv tells pmcp what it ASKED FOR, not what npm CHOSE. B2: the plan changed only the 'current' source. Both notice writers still take 'latest' from _get_server_config_for_update, which prefers the manifest and ignores the .mcp.json override -- so B's installed version can still be compared against A's upstream latest, which is the original #150 defect intact. Seven attempts have now failed at the same joint: pmcp cannot observe which package artifact is actually executing for a running server. * chore(plan): ask the server its own version (#150, eighth attempt) Maintainer decision after the spawn-provenance plan was rejected. Running the package's own --version is the only method that observes the artifact that actually runs, because the artifact answers for itself. Measured against 12 real manifest servers before planning, not assumed: 6/12 (50%) parsed a version @circleci/mcp-server-circleci IGNORED the flag and hit the 90s cap @eslint/mcp printed 'server is running' and EXITED 0 -- exit status is not a usability signal firecrawl-mcp emitted only npm noise containing koa-router@14.0.0, exactly what a permissive regex would grab as a version Strict whole-line matching verified to extract 4.0.2 and 0.0.79 while rejecting both bad cases. Median latency 5s, max 90s -> a 20s timeout and bounded concurrency are load-bearing, and ~390s serial across 98 servers makes caching required rather than an optimisation. Carries both prior board findings forward: no fallback to descriptions_cache.version (B1), and 'latest' must come from the same effective config as the probe so an override cannot be compared against the manifest package's latest (B2). Open question recorded for the board: the probe STARTS the package, so a server that ignores --version briefly runs with whatever side effects it has at startup. If that is unacceptable the honest conclusion may be to remove the feature rather than make it 50% correct. * chore(plan): measure the probe's startup side effects -- 7 procs, 5 sockets Probing @circleci/mcp-server-circleci --version (a server that ignores the flag) spawns 7 processes and opens 5 network sockets, sustained until killed. That is a real server starting and connecting outward, caused by an advisory update check, for a server that returns no version anyway. @eslint/mcp by contrast exits before 10s, so its cost is bounded. Recorded because it is the strongest argument against this approach and the maintainer asked for the board's view on exactly this point. * chore(plan): board rejects the probe 3/3 and recommends removing the feature Both readable legs answered the disqualifying question directly. grok: 'Automatic --version probes are disqualifying. Remove the automatic notices rather than ship a 50%-correct scheduled spawn.' codex: 'It executes third-party server code merely to produce an advisory.' Both rejected the 'probe only running servers' mitigation as relocating rather than removing the hazard -- those servers are the ones most likely to hold live credentials. Matches the measurement taken before the verdicts: one flag-ignoring server spawned 7 processes and 5 sockets. Separately, grok found and I verified a LIVE bug in shipped code: _run_update_probe_command spawns without start_new_session and its 60s wait_for abandons the child on timeout -- no kill, no process-group cleanup -- so today's gateway.update_server can orphan a whole process tree. Independent of #150; to be filed separately. * wip: reap the update probe's process tree on timeout (pre-test) * test: prove the update probe reaps its process tree on timeout * docs: CHANGELOG for the update-probe orphan fix * fix: catch asyncio.TimeoutError so the probe is reaped on Python 3.10 asyncio.TimeoutError only became an alias of the builtin TimeoutError in 3.11. This project supports 3.10, where 'except (TimeoutError, ...)' does not catch what wait_for raises -- so the timeout escaped and _terminate_process_tree never ran. The orphan fix silently did nothing on the oldest supported version. CI caught it: test (3.10) failed while 3.11 and 3.12 passed, and the runner log showed 'Terminate orphan process: pid (3919) (python)' -- my own regression test leaking the processes it spawns. Reproduced locally on 3.10, then verified all three versions pass with the fix. * test: run the orphan proof in a child interpreter The test must spawn real process trees -- a mock cannot demonstrate an orphan. But doing that inside the pytest process perturbs state neighbouring tests measure: tests/runtime/test_downstream_remote.py asserts on THIS process's open socket count and on the process-global sse_starlette.AppStatus latch. My test knocked over two different tests on CI's 3.10 lane while passing locally. Moves the scenario into a subprocess, so the real-process proof survives without touching the suite's shared state. Stdlib subprocess -- no test-runner plugin and no new dependency for one test. Verified: reverting the fix makes it report 'ORPHANED' (RED); with the fix 'REAPED'; and test_tools.py + test_downstream_remote.py together now pass 188/188 on 3.10, the combination that was failing.
Merged
ViperJuice
added a commit
that referenced
this pull request
Aug 21, 2026
1. _semver_key replaced by the semver library (new dep, zero transitive deps). Verified it rejects every case the hand-written regex was built to reject (leading zeros, empty identifiers, non-SemVer) and orders beta.2 < beta.11. 2. is_version_newer's docstring now warns that a bare `not is_version_newer(...)` is unsafe, and an AST-based test enforces it repo-wide -- proven to fire by removing the real guard in refresher.py. 3. All-numeric digest disambiguation. Fail-closed was tried and REVERTED: it broke CalVer, which #155 deliberately supports and pins with an existing test -- trading a live capability for a latent bug. The partner value disambiguates instead. Also fixed a real bug found on the way: the digest pattern demanded a hex letter even when the `sha256:` prefix was present, rejecting all-numeric prefixed digests. 4. The changelog CI guard falls back to the frozen event payload rather than reading a failed lookup as 'no label'. Payload passed via env, never interpolated into the script body. Fallback logic tested locally across eight cases including malformed and null JSON.
This was referenced Aug 21, 2026
ViperJuice
added a commit
that referenced
this pull request
Aug 22, 2026
* fix: close the four #155 follow-ups (#156) 1. _semver_key replaced by the semver library (new dep, zero transitive deps). Verified it rejects every case the hand-written regex was built to reject (leading zeros, empty identifiers, non-SemVer) and orders beta.2 < beta.11. 2. is_version_newer's docstring now warns that a bare `not is_version_newer(...)` is unsafe, and an AST-based test enforces it repo-wide -- proven to fire by removing the real guard in refresher.py. 3. All-numeric digest disambiguation. Fail-closed was tried and REVERTED: it broke CalVer, which #155 deliberately supports and pins with an existing test -- trading a live capability for a latent bug. The partner value disambiguates instead. Also fixed a real bug found on the way: the digest pattern demanded a hex letter even when the `sha256:` prefix was present, rejecting all-numeric prefixed digests. 4. The changelog CI guard falls back to the frozen event payload rather than reading a failed lookup as 'no label'. Payload passed via env, never interpolated into the script body. Fallback logic tested locally across eight cases including malformed and null JSON. * fix: clear the sse_starlette AppStatus latch on entry, not only exit (#158) Root cause of the intermittent test_ec_p2_7_reconnect_does_not_leak_transports failure, confirmed empirically rather than inferred: latched=False: connect OK latched=True: FAILED -> Exception: SSE stream ended without a response which is the exact error CI reported. AppStatus.should_exit is a process-global class attribute latched True by uvicorn's shutdown handler and never reset. run_fake_remote cleared it in its finally block, which protects against its OWN shutdown but not against a server stopped elsewhere in the interpreter -- tests/mcp2x/test_listen_over_http.py and tests/test_http_dos.py both stop uvicorn servers, and tests/mcp2x sorts immediately before tests/runtime. That is why CI failed at ~3% of the suite and why it never reproduced in isolation. Clearing on entry makes the server immune to whatever ran before it. Added a test that latches the flag deliberately and asserts a connection still works; proven RED without the entry reset. #158 suggested instrumenting the assertion to learn which descriptors were open. That was the right next step for a socket-count failure, but the failures were the SSE stream dying, not the count. * fix: address all three board findings on #156 1. Partner promotion REVERTED. Promoting an all-numeric value to a digest because its partner is one resolves the ambiguity by guessing, and the guess fabricates: a genuine CalVer paired with a digest reported an update that never happened. That is precisely what is_version_newer's fail-closed contract exists to prevent. Mixed pairs stay incomparable; a new test pins that every caller passes package_type, which is what actually resolves it. The separate sha256:-prefix fix is retained -- that one was unambiguous. 2. The AST negation test checked only CO-OCCURRENCE. It now requires the guard to be a SIBLING operand of the and-chain, positioned BEFORE the negation, and about the SAME value. My first strengthening still accepted the reviewer's bypass, because walking into each operand finds a guard nested in an `or`. Verified against four bypasses: or-nesting, wrong subject, no guard, and guard-after-negation. All four now fail; real code passes. 3. The CI label fallback branched on EMPTINESS, conflating "lookup failed" with "lookup succeeded, zero labels". A removed skip-changelog label would have been resurrected from the frozen payload and silently bypassed the guard -- worse than the false block the fallback was added to prevent. It now branches on exit status. Tested across five cases including label-removed. * fix: make the negation guard model control flow, not syntax Third attempt at this check, and both reviewers found the same remaining hole: (ready or is_version_orderable(x)) and not is_version_newer(x, y) passed, because the guard was searched for RECURSIVELY inside each preceding and-operand -- so a guard nested in an `or` counted, even though `ready` being true skips it entirely. The previous version also REJECTED a genuinely safe guard in an enclosing `if`, confirming it modelled syntax rather than effect. Now computes the conditions that must actually hold where the negation is evaluated: conjuncts of preceding operands in an enclosing `and` chain, plus conjuncts of an enclosing `if` test when the negation is in its body (not its orelse). An `or` contributes only itself, never its branches. Verified against five bypasses (or-nested guard in a preceding operand, `not newer or guard`, wrong subject, no guard, guard after the negation) and four legitimate shapes (bool()-wrapped, parenthesised negation, enclosing `if`, `if ready and guard`). Also rejects a guard that only holds in the `if`'s else branch. Plus the stale prose the reviewer flagged: the digest test's class docstring still described the reverted partner-promotion design, the workflow comments still described the replaced empty-check fallback, the CHANGELOG had picked up a duplicate '### Fixed' heading, and its CI bullet omitted that a SUCCESSFUL empty lookup stays authoritative. * test: accept a walrus-assigned guard, document the early-exit limitation Found while pre-attacking the review questions: (ok := is_version_orderable(v)) is truthy exactly when the guard is, so it should count -- NamedExpr is now stripped alongside bool() and double negation. An early-exit guard (if not is_version_orderable(v): return False) is genuinely safe but still trips this check. Documented in the test rather than left for someone to rediscover, with instructions to restructure or extend the analysis rather than delete the assertion. * fix: guard BOTH sides of the version comparison (#156 item 2, for real) Board review proved the item-2 defect was still live after three rounds of fixing the lint that was supposed to police it. The guard checked only the CACHED side: is_version_orderable('1.0.0') = True is_version_newer('1.0.0','nightly','npm') = False (fails closed) guard and not newer = True -> "up to date" so an unreadable FETCHED version pinned the stale cache exactly as the original bug did. Both sides are now checked, and both checks carry pkg_type -- the type is what makes an all-numeric docker digest orderable at all. Added a behavioural regression test asserting the observable (refresh_server calls get_package_version a second time on the refresh path, so a short-circuit is detectable), proven RED against the one-sided guard. The lint now requires both operands guarded and reports which one is unguarded; also proven RED. The lint remains unsound by construction -- it matches syntax, so it accepts a guarded value that is reassigned before use, or a guard whose scope ended before a closure runs. Documented in the test rather than left implicit, and filed as #164: replacing the fail-closed boolean with a tri-state result makes the hazard unrepresentable and this lint unnecessary. * fix: comparability is a property of the PAIR, not of each value (#156 item 2) Fifth board finding, and the deepest. Two unary orderability guards do not prove a pair can be ordered: is_version_orderable('1.0.0', 'docker') = True is_version_orderable('abcdef123456','docker') = True is_version_newer('1.0.0','abcdef123456','docker') = False (mixed, fails closed) -> guard passes, negation reports "up to date" A version and a digest are each orderable alone but mutually incomparable, so the short-circuit returned the stale npm cache for a server now configured for docker. Reachable because refresh_all reuses a cache entry by server NAME and never checks the package still matches. Adds are_versions_comparable(current, latest, package_type), which mirrors is_version_newer's own branching, and switches the refresher to it. The lint now requires that pair predicate over the same two operands rather than unary guards; both it and a new behavioural test are proven RED against the two-unary-guard version. is_version_orderable is no longer used in src/. Kept for genuinely unary questions, but its docstring -- and is_version_newer's, which still advised gating on it -- now say plainly that it is NOT sufficient to guard the negation. The pair/unary drift test initially failed on ('abcdef123456', 'sha256:abcdef123456'): the same image in two spellings is comparable and EQUAL, so neither direction is newer. My premise was wrong, not the code. It now asserts the invariant that actually matters -- an incomparable pair is never reported newer in either direction. * test: enforce package_type in the guard, and test the dangerous drift direction Board findings 6 and 7, both correct. 1. The lint compared only the first two operands, so a guard that DROPPED package_type passed while the comparator beside it used one. The type decides classification -- are_versions_comparable("202612180000","1.0.0") is True while the same pair as docker is incomparable -- so that guard would have recreated the unsafe short-circuit. All three operands must now match. 2. The drift test SKIPPED every pair the predicate accepts, so it could only catch "predicate rejects something orderable" (which suppresses updates). The dangerous direction is the opposite: predicate accepts a pair the comparator cannot order, which is exactly what lets `not is_version_newer(...)` read "up to date". Now tested over the same 28-value x 7-type corpus: an accepted pair must either order in one direction or be canonically the SAME value. Writing (2) exposed two bugs in my own canonical-identity helper before it passed, both of which reported drift that was not there: - comparing rendered strings, so 1.0 vs 1.0.0 (the same PEP 440 release) looked different; - then comparing full Version objects, so 1.0.0 vs 1.0.0+b looked different, when is_version_newer deliberately compares .public because build metadata is not a new release. Proven RED by injecting drift into are_versions_comparable: the test names ('1.0.0','abcdef123456',None) as comparable-but-unorderable.
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prerequisite for #150. Independently useful and independently testable, so it lands on its own.
The bug
is_version_newerextracted every digit from both strings and compared the results, with no way to signal "this is not a version". ReturningTruemakes the gateway tell an operator their server is out of date, so an unreadable version could only ever produce a false claim. Reproduced onmain:Each of those emits an "update available" notice that is not true.
The empty-string case is reachable in ordinary use, not a contrived input:
MCPServer.__init__in the current MCP SDK defaultsversionto"", and FastMCP 1.x reported the SDK version rather than the package's. Both were already documented in this repo (plans/phase-plan-v11-PG.md:44).The fix
Recognise a release number specifically —
1.2.3,1.0,2025.12.18, avprefix, and pre-release/build suffixes like-rc1or+build.5— and report no update when either side is not one.Two subtleties worth calling out:
build-1parses to(1,)under a digits-anywhere scan and compares as older than every real release. The predicate has to anchor on a leading numeric release component, or this specific fabrication survives. My first attempt at this fix got it wrong and the behaviour matrix caught it.get_docker_versionreturns asha256:digest, which is version-shaped by no definition. Digests are now compared for inequality — a different digest genuinely is a new image, while ordering hex is meaningless — so the docker lane keeps working instead of being silenced by the stricter rule.Existing documented behaviour is preserved, including
1.0.0-rc1->1.0.0-rc2=Trueand thetest_non_numeric_versionscontract thatalpha/betaare mutually not-newer.Verification
pytest -q-> 2541 passed, 3 skipped, 0 failed (2536 baseline + 5 new tests)ruff check .,ruff format --check src/ tests/,mypy src— cleantest_version_checker.pytests still pass, unmodifiedsrc/pmcp/manifest/version_checker.pyfails 3 of the new tests, includingis_version_newer('1.0.0', 'sha256:abc123') -> TrueWhy this is separate from #150
#150 needs a version source whose identity is bound to the package being compared — five attempts have failed at that, and the analysis is recorded across two plan artifacts and a closed PR (#154). But whatever source is chosen, it will fabricate notices while the comparator cannot fail. This makes the rest sound.