From 726698e2b0d4ffad4401ad135998233f123746db Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Wed, 5 Aug 2026 01:09:31 +0900 Subject: [PATCH] fix(release): validate explicit versions and drop duplicate comparators Review findings on #7586: - validate an explicit release version before comparing; the shared comparator never throws, so 999.bad previously reached every manifest - ci-release-notes imports the comparator by relative path: the release_github job runs without bun install - route Bun cache pruning through compareVersions and delete compareSemverLikeVersions - regression test for the release-version guard --- packages/coding-agent/CHANGELOG.md | 7 +++ packages/coding-agent/src/cli/update-cli.ts | 38 +------------- packages/utils/CHANGELOG.md | 4 ++ scripts/ci-release-notes.ts | 2 +- scripts/release.test.ts | 36 ++++++++++++++ scripts/release.ts | 55 ++++++++++++++------- 6 files changed, 87 insertions(+), 55 deletions(-) create mode 100644 scripts/release.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 22a72debb..94b193953 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -7,6 +7,13 @@ ### Changed - Upgraded the bundled omptype schema engine: intersection and pipe operators, bigint and RegExp literals in the string DSL, Standard Schema V1 interop, JSON Schema import via fromJsonSchema(), and richer union/collection error reporting. +### Breaking Changes + +- Renamed `compareVersions` to `compareChangelogEntries` in `@oh-my-pi/pi-coding-agent/utils/changelog`. The function signature and behavior are unchanged; update imports to use the new name. + +### Changed + +- Routed Bun install-cache pruning in `update-cli` through the shared `compareVersions` utility (`@oh-my-pi/pi-utils`), removing a duplicate local comparator that rounded large numeric version identifiers via `Number`. ## [17.2.7] - 2026-08-03 diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index ac2e2cd60..9cfc95413 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -514,42 +514,6 @@ function stripBunCacheVersionSuffix(name: string): string { return metadataIndex === -1 ? name : name.slice(0, metadataIndex); } -function compareSemverIdentifier(a: string, b: string): number { - const aNumber = /^\d+$/.test(a); - const bNumber = /^\d+$/.test(b); - if (aNumber && bNumber) return Number(a) - Number(b); - if (aNumber) return -1; - if (bNumber) return 1; - return a.localeCompare(b); -} - -function compareSemverLikeVersions(a: string, b: string): number { - const [aCoreWithPrerelease] = a.split("+", 1); - const [bCoreWithPrerelease] = b.split("+", 1); - const [aCore, aPrerelease] = aCoreWithPrerelease.split("-", 2); - const [bCore, bPrerelease] = bCoreWithPrerelease.split("-", 2); - const aParts = aCore.split("."); - const bParts = bCore.split("."); - for (let i = 0; i < Math.max(aParts.length, bParts.length); i++) { - const diff = Number(aParts[i] ?? 0) - Number(bParts[i] ?? 0); - if (diff !== 0 && Number.isFinite(diff)) return diff; - } - if (!aPrerelease && !bPrerelease) return 0; - if (!aPrerelease) return 1; - if (!bPrerelease) return -1; - const aPrereleaseParts = aPrerelease.split("."); - const bPrereleaseParts = bPrerelease.split("."); - for (let i = 0; i < Math.max(aPrereleaseParts.length, bPrereleaseParts.length); i++) { - const aPart = aPrereleaseParts[i]; - const bPart = bPrereleaseParts[i]; - if (aPart === undefined) return -1; - if (bPart === undefined) return 1; - const diff = compareSemverIdentifier(aPart, bPart); - if (diff !== 0) return diff; - } - return 0; -} - async function readdirIfExists(dir: string): Promise { try { return await fs.promises.readdir(dir, { withFileTypes: true }); @@ -671,7 +635,7 @@ export async function pruneBunInstallCache( scannedPackages++; let latestVersion: string | undefined; for (const version of group.actualDirs.keys()) { - if (!latestVersion || compareSemverLikeVersions(version, latestVersion) > 0) latestVersion = version; + if (!latestVersion || compareVersions(version, latestVersion) > 0) latestVersion = version; } if (!latestVersion) continue; for (const [version, paths] of group.actualDirs) { diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index c195b1d2c..8761240b5 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Added a public `compareVersions` utility (`@oh-my-pi/pi-utils`) that compares two version strings with SemVer-2.0 prerelease ordering, build-metadata stripping, and numeric segment comparison without float overflow; never throws. + ## [17.2.6] - 2026-08-03 ### Added diff --git a/scripts/ci-release-notes.ts b/scripts/ci-release-notes.ts index 51bc8c9bb..0f2b7d1fa 100755 --- a/scripts/ci-release-notes.ts +++ b/scripts/ci-release-notes.ts @@ -29,8 +29,8 @@ * underneath; this only adds curated context. */ -import { compareVersions } from "@oh-my-pi/pi-utils"; import { $, Glob } from "bun"; +import { compareVersions } from "../packages/utils/src/version"; const changelogGlob = new Glob("packages/*/CHANGELOG.md"); const REPO = process.env.OMP_REPO ?? process.env.GITHUB_REPOSITORY ?? "can1357/oh-my-pi"; diff --git a/scripts/release.test.ts b/scripts/release.test.ts new file mode 100644 index 000000000..cc820cb76 --- /dev/null +++ b/scripts/release.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, test } from "bun:test"; +import { isValidExplicitVersion } from "./release"; + +describe("isValidExplicitVersion", () => { + test("rejects malformed versions", () => { + expect(isValidExplicitVersion("999.bad")).toBe(false); + expect(isValidExplicitVersion("17")).toBe(false); + expect(isValidExplicitVersion("17.2")).toBe(false); + expect(isValidExplicitVersion("17.2.8.9")).toBe(false); + expect(isValidExplicitVersion("v17.2.8.9")).toBe(false); + expect(isValidExplicitVersion("abc")).toBe(false); + expect(isValidExplicitVersion("")).toBe(false); + expect(isValidExplicitVersion("v")).toBe(false); + expect(isValidExplicitVersion("17.2.8-")).toBe(false); + }); + + test("accepts valid three-segment numeric versions", () => { + expect(isValidExplicitVersion("17.2.8")).toBe(true); + expect(isValidExplicitVersion("0.0.0")).toBe(true); + expect(isValidExplicitVersion("1.0.0")).toBe(true); + }); + + test("accepts leading v prefix", () => { + expect(isValidExplicitVersion("v17.2.8")).toBe(true); + expect(isValidExplicitVersion("V17.2.8")).toBe(false); + }); + + test("accepts prerelease suffixes", () => { + expect(isValidExplicitVersion("17.2.8-rc.1")).toBe(true); + expect(isValidExplicitVersion("v17.2.8-rc.1")).toBe(true); + expect(isValidExplicitVersion("1.0.0-beta")).toBe(true); + expect(isValidExplicitVersion("1.0.0-alpha.1.2")).toBe(true); + expect(isValidExplicitVersion("1.0.0-0.3.7")).toBe(true); + expect(isValidExplicitVersion("1.0.0-x.7.z.92")).toBe(true); + }); +}); diff --git a/scripts/release.ts b/scripts/release.ts index 790c1f01f..4e79926b8 100755 --- a/scripts/release.ts +++ b/scripts/release.ts @@ -15,6 +15,14 @@ import { runChangelogFixer } from "./fix-changelogs"; const changelogGlob = new Glob("packages/*/CHANGELOG.md"); const packageJsonGlob = new Glob("packages/*/package.json"); const cargoTomlGlob = new Glob("crates/*/Cargo.toml"); +/** + * Strict explicit-version guard: three numeric dot-segments, optional leading + * `v`, optional SemVer-2.0 prerelease suffix. Bump keywords (major/minor/patch) + * are handled separately and must not be routed through this check. + */ +export function isValidExplicitVersion(version: string): boolean { + return /^v?\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$/.test(version); +} function git(args: readonly string[]) { return $`git -c core.fsmonitor=false -c core.untrackedCache=false -c fetch.pruneTags=false ${args}`; @@ -189,6 +197,17 @@ function bumpVersion(current: string, bump: "major" | "minor" | "patch"): string async function cmdRelease(versionOrBump: string): Promise { console.log("\n=== Release Script ===\n"); + // Validate explicit versions before any compare: the shared compareVersions + // never throws, so without this guard garbage like "999.bad" would be + // accepted and written into every package.json / Cargo.toml / tag. + if (versionOrBump !== "major" && versionOrBump !== "minor" && versionOrBump !== "patch") { + if (!isValidExplicitVersion(versionOrBump)) { + console.error( + `Error: Invalid version "${versionOrBump}". Expected a semver like 17.2.8 or v17.2.8-rc.1, or a bump keyword (major/minor/patch).`, + ); + process.exit(1); + } + } // 1. Pre-flight checks console.log("Pre-flight checks..."); @@ -390,23 +409,25 @@ async function cmdRelease(versionOrBump: string): Promise { // Main // ============================================================================= -const arg = process.argv[2]; +if (import.meta.main) { + const arg = process.argv[2]; -if (!arg) { - console.error("Usage:"); - console.error(" bun scripts/release.ts Full release"); - console.error(" bun scripts/release.ts watch Watch CI for current commit"); - process.exit(1); -} + if (!arg) { + console.error("Usage:"); + console.error(" bun scripts/release.ts Full release"); + console.error(" bun scripts/release.ts watch Watch CI for current commit"); + process.exit(1); + } -if (arg === "watch") { - await cmdWatch(); -} else if (arg === "major" || arg === "minor" || arg === "patch" || /^\d+\.\d+\.\d+$/.test(arg)) { - await cmdRelease(arg); -} else { - console.error(`Unknown command or invalid version: ${arg}`); - console.error("Usage:"); - console.error(" bun scripts/release.ts Full release"); - console.error(" bun scripts/release.ts watch Watch CI for current commit"); - process.exit(1); + if (arg === "watch") { + await cmdWatch(); + } else if (arg === "major" || arg === "minor" || arg === "patch" || isValidExplicitVersion(arg)) { + await cmdRelease(arg); + } else { + console.error(`Unknown command or invalid version: ${arg}`); + console.error("Usage:"); + console.error(" bun scripts/release.ts Full release"); + console.error(" bun scripts/release.ts watch Watch CI for current commit"); + process.exit(1); + } }