Conversation
…rsion listVersions() filtered stored version strings with strict semver.valid(), which requires a full X.Y.Z string. A version stored as "1.20" (major.minor, e.g. Cilium's own docs versioning scheme) failed that check and was silently dropped, making it permanently invisible to findBestVersion() even when explicitly requested. Separately, semver.maxSatisfying() also requires every candidate to itself be a fully valid semver string, so even after accepting "1.20" into the version list it still wouldn't match against a requested range. Fix both: filter with semver.coerce() so coercible partial versions are kept (non-versions like "stable"/"latest" correctly still fail coercion and stay excluded), and match on coerced candidates in findBestVersion(), mapping the winner back to its original stored string so downstream lookups use what's actually in the store. Related to but broader than arabold#475, which reports non-semver labels like "latest" specifically; this also covers legitimate partial version formats that arabold#475 doesn't. Fixes arabold#480
arabold
left a comment
There was a problem hiding this comment.
Good changes. Thanks for the report and fix. Two comments below as this breaks prerelease versions and can cause conflicts. Probably rare, but should be easy enough to include.
| // then map the winner back to its original stored string so downstream | ||
| // lookups (e.g. checkDocumentExists) use the string that's actually in | ||
| // the store. | ||
| const coercedToOriginal = new Map<string, string>(); |
There was a problem hiding this comment.
Could we avoid keeping just one original label for each normalized version? If both a partial version and its full form are stored, one replaces the other here, and looking up the partial form can return a different version label. Please add coverage for both stored forms.
Something like this:
type VersionCandidate = {
stored: string;
normalized: string;
strict: boolean;
};| // the store. | ||
| const coercedToOriginal = new Map<string, string>(); | ||
| for (const v of versionStrings) { | ||
| const coerced = semver.coerce(v); |
There was a problem hiding this comment.
This destroys prerelease labels (2.0.0-beta becomes 2.0.0), so a request for the prerelease can fail or it can win over the real stable release. A small regression test covering both would help.
Follow-up to the partial-semver fix, addressing review feedback on arabold#481. Normalizing stored version labels with semver.coerce() alone lost information in two ways: - Only one stored label survived per normalized version. With both "1.20" and "1.20.0" indexed, whichever was seen last won, so asking for one could return the other and downstream lookups keyed off a label the caller never requested. - coerce() strips prerelease tags, so "2.0.0-beta" normalized to "2.0.0". A request for the prerelease then failed outright, and the prerelease could win "latest" over the real 2.0.0 release. Introduce a VersionCandidate ({ stored, normalized, strict }) in utils/version, built by preferring strict semver and only falling back to coercion with includePrerelease. findBestVersion() now matches on the normalized forms and resolves the winner back to a stored label, preferring the caller's exact request and then a strict label, so each stored form resolves to itself. compareVersionsDescending() shares the same normalization, keeping sort order and version resolution aligned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Fixes #480 — partial semver versions like
"1.20"are permanently unmatchable viafind_version/search_docs, even thoughfindBestVersion()'s own target-matching logic already treats this format as valid (itsversionRegexmatches1.2/1, and thesearch_docsMCP tool's own description documents5.x/5.2.xX-range examples as supported input).Related to but broader than #475, which reports genuinely non-semver labels (
"latest") specifically — this covers real, common version formats a lot of projects actually use (major.minor with no patch — e.g. Cilium versions its own docs as1.20, not1.20.0).Root cause
Two independent strictness bugs, both needed fixing:
listVersions()filtered withsemver.valid(), which requires a fullX.Y.Zstring.semver.valid("1.20")→null, so a version stored literally as"1.20"was silently dropped and never even reachedfindBestVersion()'s matching logic.semver.maxSatisfying()also requires every candidate to itself be a fully validX.Y.Zstring —semver.maxSatisfying(["1.20"], "1.20")→null. So even after fixing Allow exceeding the maximum chunk size if a chunk cannot be split further #1, matching would still fail without coercing candidates first.Repro
Indexed
ciliumundernull(unversioned),"1.20", and"stable". Before the fix:"1.20"was completely unreachable via its own version string despite having been successfully indexed.search_docsmasked this by silently falling back to the unversioned bucket regardless of theversionparam passed (SearchTool.ts,exactMatchhardcodedfalsefor the MCP interface) — which is also why this can present as an outright"Library not found in store"failure (matching #475's report) whenever no unversioned copy happens to exist to fall back to.Fix
listVersions(): filter withsemver.coerce(v) !== nullinstead ofsemver.valid(v).findBestVersion(): build acoerced -> originalmap, match on the coerced candidates, then resolve back to the original stored string (so downstream lookups likecheckDocumentExists()still use what's actually in the store).Test plan
listVersions(): partial versions ("1.20","5") are now included and sort correctly alongside full versions; non-versions ("stable") stay excludedfindBestVersion(): resolves an explicit"1.20"request to the original stored string; picks the highest partial version when no target is given; a non-semver label like"stable"still correctly falls through to unversioned (unchanged, correct behavior)npm run lint,npm run typecheck,npx vitest run src/store/DocumentManagementService.test.tsall passcilium@1.20scrape with a local build:find_versionnow returns"Best match: 1.20"instead of failing