From 97df6a2c93617d8008e82e66e3980d0dd90054e8 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 11:29:39 +0100 Subject: [PATCH] fix(mcp): sort versions on a stored key, and stop shadowing the latest scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scopeOrderByVersion sorted with three nested SUBSTRING_INDEX calls. That is MySQL-only, so the scope could not execute on sqlite at all and nothing that ordered versions was testable. It also interpolated $direction straight into the raw string. Versions now carry a normalised version_sort key, written on save and compared as a plain string: each component zero-padded to five digits so "10.0.0" sorts above "9.9.9", and the pre-release suffix appended after a separator with "~" (0x7E, above every alphanumeric) standing in for "no suffix" so 1.0.0 outranks 1.0.0-beta as semver requires. Parsing happens once at write time instead of in every query, and the human-readable `version` column stays the display truth. Ordering is now a plain indexed ORDER BY and $direction is whitelisted to asc/desc rather than interpolated. The migration backfills existing rows with chunkById, not chunk. The filter is whereNull('version_sort') and the loop fills that same column in, so every processed row leaves the result set; chunk() pages with OFFSET and would skip a page's worth for each page written. Measured on 1200 rows: chunk() left 500 of them null, chunkById left none. Fixing the ordering exposed a second defect underneath it. The model defines scopeLatest, but Illuminate's query builder already has latest(), and a real method always beats a local scope — so scopeLatest was never once called. Every caller meaning "the version flagged is_latest" silently got orderBy('created_at', 'desc'), which filters nothing and, for rows created in the same second, does not even order deterministically. That is why getLatestVersion returned 1.0.0 after 2.0.0 had been explicitly marked latest. Renamed to markedLatest so it cannot be shadowed, and the five call sites that meant it are updated: getLatestVersion, ToolRegistry's version enrichment, the two deprecation/sunset suggestions on the model, and the admin ToolVersionManager filter — where status 'latest' sat beside deprecated() and sunset() and had been filtering nothing at all. ToolVersionServiceTest is now fully green, including the three that failed before. Suite: 21 failed, 299 passed, from 25 failed, 289 passed. Six new tests cover the key itself — padding, semver ordering, pre-release precedence, missing components, malformed input, and that it follows a corrected version rather than going stale. Co-Authored-By: Virgil --- ..._add_version_sort_to_mcp_tool_versions.php | 74 ++++++++++++++ php/src/Mcp/Models/McpToolVersion.php | 94 +++++++++++++++--- php/src/Mcp/Services/ToolRegistry.php | 2 +- php/src/Mcp/Services/ToolVersionService.php | 2 +- .../View/Modal/Admin/ToolVersionManager.php | 2 +- php/tests/Unit/VersionSortKeyTest.php | 98 +++++++++++++++++++ 6 files changed, 258 insertions(+), 14 deletions(-) create mode 100644 php/src/Mcp/Migrations/2026_08_08_000001_add_version_sort_to_mcp_tool_versions.php create mode 100644 php/tests/Unit/VersionSortKeyTest.php diff --git a/php/src/Mcp/Migrations/2026_08_08_000001_add_version_sort_to_mcp_tool_versions.php b/php/src/Mcp/Migrations/2026_08_08_000001_add_version_sort_to_mcp_tool_versions.php new file mode 100644 index 0000000..84dc5fc --- /dev/null +++ b/php/src/Mcp/Migrations/2026_08_08_000001_add_version_sort_to_mcp_tool_versions.php @@ -0,0 +1,74 @@ +string('version_sort', 64)->nullable()->after('version'); + + $table->index(['server_id', 'tool_name', 'version_sort'], 'mcp_tool_versions_sort'); + }); + } + + // Backfill existing rows, chunked rather than loaded whole: this table + // grows a row per tool per release and a deployed instance may hold a + // lot of them. + // + // chunkById, not chunk: the filter is whereNull('version_sort') and the + // loop fills that same column in, so each processed row leaves the + // result set. chunk() pages with OFFSET, so the set shrinking under it + // would skip a whole page's worth of rows for every page written — + // silently backfilling roughly half the table. chunkById pages with + // `where id > `, which is stable under that mutation. + DB::table('mcp_tool_versions') + ->select('id', 'version') + ->whereNull('version_sort') + ->chunkById(500, function ($rows) { + foreach ($rows as $row) { + DB::table('mcp_tool_versions') + ->where('id', $row->id) + ->update(['version_sort' => McpToolVersion::versionSortKey((string) $row->version)]); + } + }); + } + + public function down(): void + { + if (! Schema::hasColumn('mcp_tool_versions', 'version_sort')) { + return; + } + + Schema::table('mcp_tool_versions', function (Blueprint $table) { + $table->dropIndex('mcp_tool_versions_sort'); + $table->dropColumn('version_sort'); + }); + } +}; diff --git a/php/src/Mcp/Models/McpToolVersion.php b/php/src/Mcp/Models/McpToolVersion.php index 7bf5bc6..24b833e 100644 --- a/php/src/Mcp/Models/McpToolVersion.php +++ b/php/src/Mcp/Models/McpToolVersion.php @@ -60,6 +60,21 @@ class McpToolVersion extends Model 'sunset_at' => 'datetime', ]; + /** + * Keep the sortable key in step with the version on every write. + * + * On saving rather than creating: a version string can be corrected after + * the fact, and a key that silently stopped matching its version would be + * worse than no key at all. version_sort is derived, never assigned by a + * caller, so it is deliberately absent from $fillable. + */ + protected static function booted(): void + { + static::saving(function (self $model): void { + $model->version_sort = static::versionSortKey((string) $model->version); + }); + } + // ------------------------------------------------------------------------- // Scopes // ------------------------------------------------------------------------- @@ -89,9 +104,19 @@ public function scopeForVersion(Builder $query, string $version): Builder } /** - * Get only latest versions. + * Get only the version flagged is_latest. + * + * Named markedLatest, not latest: Illuminate's query builder already + * defines latest(), and a real method always wins over a local scope, so + * scopeLatest() was never once called. Every caller that meant "the marked + * latest version" silently got orderBy('created_at', 'desc') instead — + * which filters nothing, and on rows created in the same second does not + * even order deterministically. + * + * @example + * McpToolVersion::forTool('plan_create')->markedLatest()->first(); */ - public function scopeLatest(Builder $query): Builder + public function scopeMarkedLatest(Builder $query): Builder { return $query->where('is_latest', true); } @@ -127,16 +152,63 @@ public function scopeActive(Builder $query): Builder /** * Order by version (newest first using semver sort). + * + * Orders on the stored version_sort key, which is written at save time and + * compares correctly as a plain string. This was three nested + * SUBSTRING_INDEX calls in raw SQL — a MySQL-only construct, so the scope + * could not run on sqlite at all and nothing that ordered versions was + * testable. It also interpolated $direction straight into the raw string. + * + * @example + * McpToolVersion::forTool('plan_create')->orderByVersion()->first(); */ public function scopeOrderByVersion(Builder $query, string $direction = 'desc'): Builder { - // Basic version ordering - splits on dots and orders numerically - // For production use, consider a more robust semver sorting approach - return $query->orderByRaw( - "CAST(SUBSTRING_INDEX(version, '.', 1) AS UNSIGNED) {$direction}, ". - "CAST(SUBSTRING_INDEX(SUBSTRING_INDEX(version, '.', 2), '.', -1) AS UNSIGNED) {$direction}, ". - "CAST(SUBSTRING_INDEX(SUBSTRING_INDEX(version, '.', 3), '.', -1) AS UNSIGNED) {$direction}" - ); + // Whitelisted, not interpolated: $direction reaches the query builder as + // a direction keyword, and anything else is a caller bug, not a value. + $direction = strtolower($direction) === 'asc' ? 'asc' : 'desc'; + + return $query->orderBy('version_sort', $direction); + } + + /** + * Build the sortable key for a semver string. + * + * Each numeric component is zero-padded to a fixed width so a plain string + * comparison matches numeric order — without padding "10.0.0" sorts before + * "9.0.0". The pre-release suffix is appended after a separator, and a + * release with no suffix gets "~" (0x7E), which sorts after every + * alphanumeric, so 1.0.0 correctly outranks 1.0.0-beta as semver requires. + * + * Non-numeric or missing components resolve to 0, so a malformed version + * still yields a comparable key rather than throwing on write. + * + * @example + * McpToolVersion::versionSortKey('1.2.3'); // '00001.00002.00003.~' + * McpToolVersion::versionSortKey('1.0.0-beta'); // '00001.00000.00000.beta' + */ + public static function versionSortKey(string $version): string + { + $version = trim($version); + + // Split the pre-release/build suffix off the numeric core. + $core = $version; + $suffix = ''; + foreach (['-', '+'] as $separator) { + $at = strpos($core, $separator); + if ($at !== false) { + $suffix = substr($core, $at + 1); + $core = substr($core, 0, $at); + } + } + + $parts = explode('.', $core); + $key = ''; + for ($i = 0; $i < 3; $i++) { + $key .= sprintf('%05d.', (int) ($parts[$i] ?? 0)); + } + + return $key.($suffix === '' ? '~' : $suffix); } // ------------------------------------------------------------------------- @@ -217,7 +289,7 @@ public function getDeprecationWarning(): ?array // Find the latest version to suggest $latest = static::forServer($this->server_id) ->forTool($this->tool_name) - ->latest() + ->markedLatest() ->first(); if ($latest && $latest->version !== $this->version) { @@ -256,7 +328,7 @@ public function getSunsetError(): ?array // Find the latest version to suggest $latest = static::forServer($this->server_id) ->forTool($this->tool_name) - ->latest() + ->markedLatest() ->first(); if ($latest && $latest->version !== $this->version) { diff --git a/php/src/Mcp/Services/ToolRegistry.php b/php/src/Mcp/Services/ToolRegistry.php index 44766e0..59f1d16 100644 --- a/php/src/Mcp/Services/ToolRegistry.php +++ b/php/src/Mcp/Services/ToolRegistry.php @@ -108,7 +108,7 @@ public function getToolsForServer(string $serverId, bool $includeVersionInfo = f if ($includeVersionInfo) { $latestVersion = McpToolVersion::forServer($serverId) ->forTool($name) - ->latest() + ->markedLatest() ->first(); if ($latestVersion) { diff --git a/php/src/Mcp/Services/ToolVersionService.php b/php/src/Mcp/Services/ToolVersionService.php index b660826..6bad2e9 100644 --- a/php/src/Mcp/Services/ToolVersionService.php +++ b/php/src/Mcp/Services/ToolVersionService.php @@ -137,7 +137,7 @@ public function getLatestVersion(string $serverId, string $toolName): ?McpToolVe // First try to find explicitly marked latest $latest = McpToolVersion::forServer($serverId) ->forTool($toolName) - ->latest() + ->markedLatest() ->first(); if ($latest) { diff --git a/php/src/Mcp/View/Modal/Admin/ToolVersionManager.php b/php/src/Mcp/View/Modal/Admin/ToolVersionManager.php index 74a862e..7d9981e 100644 --- a/php/src/Mcp/View/Modal/Admin/ToolVersionManager.php +++ b/php/src/Mcp/View/Modal/Admin/ToolVersionManager.php @@ -99,7 +99,7 @@ public function versions(): LengthAwarePaginator } if ($this->status === 'latest') { - $query->latest(); + $query->markedLatest(); } elseif ($this->status === 'deprecated') { $query->deprecated(); } elseif ($this->status === 'sunset') { diff --git a/php/tests/Unit/VersionSortKeyTest.php b/php/tests/Unit/VersionSortKeyTest.php new file mode 100644 index 0000000..0964156 --- /dev/null +++ b/php/tests/Unit/VersionSortKeyTest.php @@ -0,0 +1,98 @@ +assertSame('00001.00002.00003.~', McpToolVersion::versionSortKey('1.2.3')); + $this->assertSame('00010.00000.00000.~', McpToolVersion::versionSortKey('10.0.0')); + } + + public function test_version_sort_key_good_orders_semver_as_plain_strings(): void + { + $versions = ['2.0.0', '1.0.0', '10.0.0', '9.9.9', '1.1.0', '0.0.1']; + + $keyed = []; + foreach ($versions as $version) { + $keyed[$version] = McpToolVersion::versionSortKey($version); + } + asort($keyed); + + // Without the zero padding this is the case that breaks: "10.0.0" + // sorts before "9.9.9" on a naive string comparison. + $this->assertSame( + ['0.0.1', '1.0.0', '1.1.0', '2.0.0', '9.9.9', '10.0.0'], + array_keys($keyed), + ); + } + + public function test_version_sort_key_good_ranks_a_release_above_its_prereleases(): void + { + $keyed = [ + '1.0.0' => McpToolVersion::versionSortKey('1.0.0'), + '1.0.0-beta' => McpToolVersion::versionSortKey('1.0.0-beta'), + '1.0.0-alpha' => McpToolVersion::versionSortKey('1.0.0-alpha'), + ]; + asort($keyed); + + // semver: a pre-release precedes the release it leads to. "~" is 0x7E, + // above every alphanumeric, which is what puts the bare release last. + $this->assertSame(['1.0.0-alpha', '1.0.0-beta', '1.0.0'], array_keys($keyed)); + } + + public function test_version_sort_key_bad_treats_missing_components_as_zero(): void + { + $this->assertSame('00001.00000.00000.~', McpToolVersion::versionSortKey('1')); + $this->assertSame('00001.00002.00000.~', McpToolVersion::versionSortKey('1.2')); + } + + public function test_version_sort_key_ugly_still_returns_a_key_for_nonsense(): void + { + // A malformed version must not throw on write — a comparable key that + // sorts low is better than a failed save. + $this->assertSame('00000.00000.00000.~', McpToolVersion::versionSortKey('')); + + // Non-numeric components resolve to zero, and anything after the first + // "-" is still read as a pre-release suffix, so garbage sorts below + // every real release rather than blowing up. + $this->assertSame('00000.00000.00000.a.version', McpToolVersion::versionSortKey('not-a.version')); + $this->assertSame('00000.00000.00000.~', McpToolVersion::versionSortKey('rubbish')); + + $this->assertLessThan( + McpToolVersion::versionSortKey('0.0.1'), + McpToolVersion::versionSortKey('rubbish'), + ); + } + + public function test_version_sort_key_good_is_written_on_save(): void + { + $version = McpToolVersion::create([ + 'server_id' => 'test-server', + 'tool_name' => 'test_tool', + 'version' => '3.4.5', + ]); + + $this->assertSame('00003.00004.00005.~', $version->fresh()->version_sort); + + // Derived, so it follows a corrected version rather than going stale. + $version->update(['version' => '4.0.0']); + + $this->assertSame('00004.00000.00000.~', $version->fresh()->version_sort); + } +}