fix(mcp): sort versions on a stored key, and stop shadowing the latest scope - #18
Merged
Conversation
…t scope
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 <virgil@lethean.io>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesThe change adds a persisted, indexed version sort key. Version sorting and latest-version selection
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ToolVersionService
participant McpToolVersion
participant Database
Caller->>ToolVersionService: request latest version
ToolVersionService->>McpToolVersion: query markedLatest()
McpToolVersion->>Database: filter is_latest and order by version_sort
Database-->>McpToolVersion: return selected version
McpToolVersion-->>ToolVersionService: return latest version
ToolVersionService-->>Caller: return version
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
The sortable stored version key, as decided. Suite: 25 failed / 289 passed → 21 failed / 299 passed.
ToolVersionServiceTestis now fully green.The ordering
scopeOrderByVersionsorted with three nestedSUBSTRING_INDEXcalls — MySQL-only, so the scope could not execute on sqlite at all and nothing that ordered versions was testable. It also interpolated$directionstraight into the raw string.Versions now carry a normalised
version_sortkey, written on save and compared as a plain string:Each component zero-padded to five digits, so
10.0.0sorts above9.9.9. The pre-release suffix follows a separator, with~(0x7E, above every alphanumeric) standing in for "no suffix" — so1.0.0outranks1.0.0-beta, as semver requires.Parsing happens once at write time instead of in every query. The human-readable
versioncolumn stays display truth; ordering is a plain indexedORDER BY;$directionis whitelisted to asc/desc.The backfill uses
chunkById, notchunkThe filter is
whereNull('version_sort')and the loop fills that same column in, so every processed row leaves the result set.chunk()pages withOFFSET, so the set shrinking underneath it skips a page's worth for each page written. Measured on 1200 rows:chunk()chunkById()The defect underneath it
Fixing the ordering exposed a second, larger one.
The model defines
scopeLatest. Illuminate's query builder already haslatest(), and a real method always beats a local scope — soscopeLatestwas never once called. Every caller meaning "the version flaggedis_latest" silently gotorderBy('created_at', 'desc'): which filters nothing, and for rows created in the same second does not even order deterministically. That is whygetLatestVersionreturned1.0.0after2.0.0had been explicitly marked latest.Renamed to
markedLatestso it cannot be shadowed. Five call sites meant it:ToolVersionService::getLatestVersionToolRegistry's version enrichmentToolVersionManagerfilter — wherestatus === 'latest'sat besidedeprecated()andsunset()and had been filtering nothing at allTests
Six new cases cover the key directly: padding, semver ordering, pre-release precedence, missing components, malformed input, and that it follows a corrected version rather than going stale.
Noted, not included
While chasing the
getLatestVersionfailure I added aCache::flush()to the baseTestCase—CACHE_STOREisarrayand lives for the whole process whileRefreshDatabaseresets the database, so a cached model can outlive the row it came from. Two suites already flush by hand in their ownsetUp. It turned out not to be the cause here and changed no result, so it is left out rather than shipped as scope creep — but the order-dependency is real and worth its own change.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Improvements
Bug Fixes