fix: follow up on #907 version edge cases (#921)

* fix: resolve rc precedence and metadata fallback drift

Ensure version coherence prefers stable releases over matching pre-release tags, and make fetchAll toolkit metadata resolution use the same providerId fallback path as single-toolkit fetches.

Made-with: Cursor

* refactor: split semver comparison into helpers

Reduce comparator complexity while preserving semver precedence behavior for stable and pre-release versions.

Made-with: Cursor

* refactor: simplify prerelease comparator branching

Extract optional prerelease part comparison so the semver helper stays readable and below complexity warning thresholds.

Made-with: Cursor

* test: expand version and metadata fallback coverage

Fix redundant metadata lookups in fetchToolkitData and add regression tests for semver prerelease ordering plus metadata lookup behavior across single and bulk toolkit paths.

Made-with: Cursor

* refactor: harden semver filter and simplify metadata resolution

Address review findings from six parallel reviewers:
- Use compareVersions for filter equality so semver-equal tools (e.g. @1.0.0 vs @1.0.0+build.1) no longer get silently dropped.
- Replace localeCompare("en") with ASCII byte ordering per SemVer 2.0.0 §11.4.2 so mixed-case prerelease tags rank correctly.
- Replace ambiguous best === "" sentinel with a null sentinel in getHighestVersion.
- Drop the allowToolkitIdLookup flag and lift the toolkit-id retry into the fetchAllToolkitsData call site; shared helper now only owns the *Api provider-id fallback.
- Inline single-use compareOptionalPrereleasePart and simplify comparePrereleaseIdentifier with typeof narrowing.
- Add regression tests: direct-map-hit no-relookup, patch-only version diff, ASCII prerelease ordering, string-vs-string prerelease, all-unversioned tools, semver-equal tools with differing build metadata.

Made-with: Cursor

Author: jottakka

toolkit-docs-generator/src/sources/toolkit-data-source.ts

@@ -108,28 +108,45 @@ export class CombinedToolkitDataSource implements IToolkitDataSource {
     this.metadataSource = config.metadataSource;
   }
 
+  /**
+   * Apply the "*Api" provider-id fallback when the direct metadata lookup
+   * missed. Used by both `fetchToolkitData` and `fetchAllToolkitsData` so
+   * they resolve metadata the same way.
+   */
+  private async resolveProviderMetadata(
+    toolkitId: string,
+    tools: readonly ToolDefinition[],
+    directMetadata: ToolkitMetadata | null
+  ): Promise<ToolkitMetadata | null> {
+    if (directMetadata || !normalizeId(toolkitId).endsWith("api")) {
+      return directMetadata;
+    }
+
+    const providerId = tools.find((t) => t.auth?.providerId)?.auth?.providerId;
+    if (!providerId) {
+      return null;
+    }
+
+    return (
+      (await this.metadataSource.getToolkitMetadata(`${providerId}Api`)) ??
+      (await this.metadataSource.getToolkitMetadata(providerId))
+    );
+  }
+
   async fetchToolkitData(
     toolkitId: string,
     version?: string
   ): Promise<ToolkitData> {
     // Fetch tools and metadata in parallel
-    const [tools, fetchedMetadata] = await Promise.all([
+    const [tools, directMetadata] = await Promise.all([
       this.toolSource.fetchToolsByToolkit(toolkitId),
       this.metadataSource.getToolkitMetadata(toolkitId),
     ]);
-
-    // If the toolkit isn't in the Design System under its exact ID, try to match
-    // based on the auth provider for "*Api" toolkits (e.g. MailchimpMarketingApi -> MailchimpApi).
-    let metadata = fetchedMetadata;
-    if (!metadata && normalizeId(toolkitId).endsWith("api")) {
-      const providerId = tools.find((t) => t.auth?.providerId)?.auth
-        ?.providerId;
-      if (providerId) {
-        metadata =
-          (await this.metadataSource.getToolkitMetadata(`${providerId}Api`)) ??
-          (await this.metadataSource.getToolkitMetadata(providerId));
-      }
-    }
+    const metadata = await this.resolveProviderMetadata(
+      toolkitId,
+      tools,
+      directMetadata
+    );
 
     // Filter tools by version if specified, otherwise keep only the highest
     // version to drop stale tools from older releases that Engine still serves.
@@ -184,10 +201,16 @@ export class CombinedToolkitDataSource implements IToolkitDataSource {
     // matching the behaviour of fetchToolkitData.
     const result = new Map<string, ToolkitData>();
     for (const [toolkitId, tools] of toolkitGroups) {
-      const directMetadata = metadataMap.get(toolkitId) ?? null;
-      const metadata =
-        directMetadata ??
+      // Prefer the batch lookup; only fall back to per-toolkit lookup
+      // (and then the provider-id fallback) when the map misses.
+      const directMetadata =
+        metadataMap.get(toolkitId) ??
         (await this.metadataSource.getToolkitMetadata(toolkitId));
+      const metadata = await this.resolveProviderMetadata(
+        toolkitId,
+        tools,
+        directMetadata
+      );
       result.set(toolkitId, { tools, metadata });
     }
 

toolkit-docs-generator/src/utils/version-coherence.ts

@@ -1,71 +1,142 @@
 import type { ToolDefinition } from "../types/index.js";
 import { extractVersion } from "./fp.js";
 
+interface ParsedSemver {
+  readonly core: readonly number[];
+  readonly prerelease: readonly (number | string)[] | null;
+}
+
 /**
- * Parse the numeric MAJOR.MINOR.PATCH tuple from a semver string,
- * stripping pre-release (`-alpha.1`) and build metadata (`+build.456`).
- *
- * Handles formats produced by arcade-mcp's normalize_version():
- *   "3.1.3", "1.2.3-beta.1", "1.2.3+build.456", "1.2.3-rc.1+build.789"
+ * Parse a semver-ish string into numeric core + optional pre-release parts.
+ * Build metadata (+...) is ignored for ordering.
  */
-const parseNumericVersion = (version: string): number[] => {
-  // Strip build metadata (after +) then pre-release (after -)
-  const core = version.split("+")[0]?.split("-")[0] ?? version;
-  return core.split(".").map((s) => {
-    const n = Number(s);
+const parseSemver = (version: string): ParsedSemver => {
+  const withoutBuild = version.split("+")[0] ?? version;
+  const prereleaseIndex = withoutBuild.indexOf("-");
+  const coreText =
+    prereleaseIndex >= 0
+      ? withoutBuild.slice(0, prereleaseIndex)
+      : withoutBuild;
+  const prereleaseText =
+    prereleaseIndex >= 0 ? withoutBuild.slice(prereleaseIndex + 1) : "";
+
+  const core = coreText.split(".").map((segment) => {
+    const n = Number(segment);
     return Number.isNaN(n) ? 0 : n;
   });
+
+  const prerelease =
+    prereleaseText.length > 0
+      ? prereleaseText.split(".").map((identifier) => {
+          const n = Number(identifier);
+          return Number.isNaN(n) ? identifier : n;
+        })
+      : null;
+
+  return { core, prerelease };
+};
+
+const compareCoreVersions = (
+  aCore: readonly number[],
+  bCore: readonly number[]
+): number => {
+  const len = Math.max(aCore.length, bCore.length);
+  for (let i = 0; i < len; i++) {
+    const diff = (aCore[i] ?? 0) - (bCore[i] ?? 0);
+    if (diff !== 0) return diff;
+  }
+  return 0;
 };
 
 /**
- * Compare two semver version strings numerically by MAJOR.MINOR.PATCH.
- * Pre-release and build metadata are ignored for ordering purposes
- * (they are unlikely to appear in Engine API responses, but we handle
- * them defensively since arcade-mcp's semver allows them).
- *
- * Returns a positive number if a > b, negative if a < b, 0 if equal.
+ * Compare two prerelease identifiers following SemVer 2.0.0 §11.4.2:
+ * numeric identifiers always rank lower than alphanumeric identifiers,
+ * numeric identifiers compare numerically, and alphanumeric identifiers
+ * compare by ASCII byte order (not locale).
  */
-const compareVersions = (a: string, b: string): number => {
-  const aParts = parseNumericVersion(a);
-  const bParts = parseNumericVersion(b);
-  const len = Math.max(aParts.length, bParts.length);
+const comparePrereleaseIdentifier = (
+  aPart: number | string,
+  bPart: number | string
+): number => {
+  if (typeof aPart === "number" && typeof bPart === "number") {
+    return aPart - bPart;
+  }
+  if (typeof aPart === "number") return -1;
+  if (typeof bPart === "number") return 1;
+  if (aPart < bPart) return -1;
+  if (aPart > bPart) return 1;
+  return 0;
+};
+
+/**
+ * Compare prerelease identifier arrays following SemVer 2.0.0 §11.4:
+ * a prerelease version has lower precedence than the associated normal
+ * version, and longer identifier lists with matching prefix rank higher.
+ */
+const comparePrerelease = (
+  aPrerelease: readonly (number | string)[] | null,
+  bPrerelease: readonly (number | string)[] | null
+): number => {
+  if (!(aPrerelease || bPrerelease)) return 0;
+  if (!aPrerelease) return 1;
+  if (!bPrerelease) return -1;
+
+  const len = Math.max(aPrerelease.length, bPrerelease.length);
   for (let i = 0; i < len; i++) {
-    const diff = (aParts[i] ?? 0) - (bParts[i] ?? 0);
+    const aPart = aPrerelease[i];
+    const bPart = bPrerelease[i];
+    if (aPart === undefined) return -1;
+    if (bPart === undefined) return 1;
+    const diff = comparePrereleaseIdentifier(aPart, bPart);
     if (diff !== 0) return diff;
   }
+
   return 0;
 };
 
+/**
+ * Compare two semver version strings using SemVer 2.0.0 precedence:
+ * - MAJOR.MINOR.PATCH compared numerically
+ * - Stable release > prerelease when core is equal
+ * - Numeric prerelease identifiers compare numerically
+ * - Alphanumeric prerelease identifiers compare by ASCII byte order
+ * - Build metadata is ignored for precedence
+ *
+ * Returns a positive number if a > b, negative if a < b, 0 if equal.
+ */
+export const compareVersions = (a: string, b: string): number => {
+  const aSemver = parseSemver(a);
+  const bSemver = parseSemver(b);
+  const coreDiff = compareCoreVersions(aSemver.core, bSemver.core);
+  if (coreDiff !== 0) return coreDiff;
+  return comparePrerelease(aSemver.prerelease, bSemver.prerelease);
+};
+
 /**
  * Find the highest version among all tools in a toolkit.
- * This is the version we keep — stale tools from older releases are dropped.
+ * Returns null when no tool carries a usable version.
  */
 export const getHighestVersion = (
   tools: readonly ToolDefinition[]
 ): string | null => {
-  if (tools.length === 0) {
-    return null;
-  }
-
-  let best = "";
+  let best: string | null = null;
   for (const tool of tools) {
     const version = extractVersion(tool.fullyQualifiedName);
-    if (best === "" || compareVersions(version, best) > 0) {
+    if (best === null || compareVersions(version, best) > 0) {
       best = version;
     }
   }
-
-  return best || null;
+  return best;
 };
 
 /**
- * Keep only tools whose @version matches the highest version for
- * their toolkit. If all tools share the same version (the common
- * case), returns the original array unchanged.
+ * Keep only tools whose @version is semver-equivalent to the highest
+ * version for their toolkit. If all tools share the highest version
+ * (the common case), returns the original array unchanged.
  *
- * This drops stale tools from older releases that Engine still serves,
- * while always preserving the newest version — even if it has fewer tools
- * (e.g. tools were removed/consolidated in the new release).
+ * Tools are compared via `compareVersions`, so build metadata is
+ * ignored when deciding equivalence — a tool at `@1.0.0+build.1`
+ * survives alongside `@1.0.0` instead of being silently dropped.
  */
 export const filterToolsByHighestVersion = (
   tools: readonly ToolDefinition[]
@@ -75,13 +146,14 @@ export const filterToolsByHighestVersion = (
     return tools;
   }
 
-  // Fast path: if every tool is already at the highest version, skip filtering
   const allSame = tools.every(
-    (t) => extractVersion(t.fullyQualifiedName) === highest
+    (t) => compareVersions(extractVersion(t.fullyQualifiedName), highest) === 0
   );
   if (allSame) {
     return tools;
   }
 
-  return tools.filter((t) => extractVersion(t.fullyQualifiedName) === highest);
+  return tools.filter(
+    (t) => compareVersions(extractVersion(t.fullyQualifiedName), highest) === 0
+  );
 };