chore: address Copilot review round 16
`parseClaudeVersion` dropped SemVer suffixes, so an installed `2.1.250-beta.1`
parsed as `[2, 1, 250]` and compared EQUAL to the stable pin — a different
validator schema running locally while the log claimed an exact match, which is
the cross-machine disagreement the pin exists to prevent.
It now returns `{ parts, prerelease }`, because the two callers want different
things: `isPinnedVersion` rejects any prerelease (build metadata still matches,
per SemVer precedence), while the eval's availability probe only needs some
usable CLI. `formatClaudeVersion` keeps the suffix in the messages, so a
prerelease is reported as what it is rather than as the pin.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
3905ceef06
commit
c26995bf82
5 changed files with 102 additions and 54 deletions
|
|
@ -253,21 +253,48 @@ export function validateEvalCases(skillName, cases) {
|
|||
export const PINNED_CLI_VERSION = "2.1.250";
|
||||
|
||||
/**
|
||||
* Read a semver triple out of `claude --version` output ("2.1.250 (Claude Code)").
|
||||
* Read the version out of `claude --version` output ("2.1.250 (Claude Code)").
|
||||
*
|
||||
* Returns the release triple **and** whether a prerelease/build suffix was
|
||||
* attached, because the two callers want different things: the pin comparison
|
||||
* must reject `2.1.250-beta.1` (a different validator schema that would compare
|
||||
* equal to the stable pin while the log claimed an exact match — Copilot),
|
||||
* while the eval's availability probe only needs *some* usable CLI.
|
||||
*
|
||||
* @param {string} text
|
||||
* @returns {number[] | null}
|
||||
* @returns {{ parts: number[], prerelease: string | null } | null}
|
||||
*/
|
||||
export function parseClaudeVersion(text) {
|
||||
const m = /(\d+)\.(\d+)\.(\d+)/.exec(text);
|
||||
return m === null ? null : [Number(m[1]), Number(m[2]), Number(m[3])];
|
||||
const m =
|
||||
/(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z.-]+))?(?:\+[0-9A-Za-z.-]+)?/.exec(text);
|
||||
if (m === null) return null;
|
||||
return {
|
||||
parts: [Number(m[1]), Number(m[2]), Number(m[3])],
|
||||
prerelease: m[4] ?? null,
|
||||
};
|
||||
}
|
||||
|
||||
/** A parsed version, formatted the way the CLI reports it. */
|
||||
export function formatClaudeVersion(version) {
|
||||
if (version === null) return "(unreadable version)";
|
||||
return (
|
||||
version.parts.join(".") +
|
||||
(version.prerelease ? `-${version.prerelease}` : "")
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {number[]} a
|
||||
* @param {number[]} b
|
||||
* @returns {number} negative if a < b, 0 if equal, positive if a > b.
|
||||
* Whether a parsed version IS the pin — same triple and no prerelease.
|
||||
*
|
||||
* @param {{ parts: number[], prerelease: string | null } | null} version
|
||||
* @param {string} pin e.g. `PINNED_CLI_VERSION`.
|
||||
*/
|
||||
export function isPinnedVersion(version, pin) {
|
||||
if (version === null || version.prerelease !== null) return false;
|
||||
const pinned = parseClaudeVersion(pin);
|
||||
return pinned !== null && compareVersions(version.parts, pinned.parts) === 0;
|
||||
}
|
||||
|
||||
export function compareVersions(a, b) {
|
||||
for (let i = 0; i < Math.max(a.length, b.length); i++) {
|
||||
const diff = (a[i] ?? 0) - (b[i] ?? 0);
|
||||
|
|
|
|||
|
|
@ -14,6 +14,8 @@ import {
|
|||
parseClaudeVersion,
|
||||
compareVersions,
|
||||
PINNED_CLI_VERSION,
|
||||
isPinnedVersion,
|
||||
formatClaudeVersion,
|
||||
} from "./skill-manifest.mjs";
|
||||
|
||||
const fm = (body) => `---\n${body}\n---\n\n# Body\n`;
|
||||
|
|
@ -203,23 +205,47 @@ test("validateEvalCases rejects malformed cases", () => {
|
|||
});
|
||||
|
||||
test("parseClaudeVersion reads the CLI's version banner", () => {
|
||||
assert.deepEqual(parseClaudeVersion("2.1.250 (Claude Code)"), [2, 1, 250]);
|
||||
assert.deepEqual(parseClaudeVersion("claude 10.0.4\n"), [10, 0, 4]);
|
||||
assert.deepEqual(parseClaudeVersion("2.1.250 (Claude Code)"), {
|
||||
parts: [2, 1, 250],
|
||||
prerelease: null,
|
||||
});
|
||||
assert.deepEqual(parseClaudeVersion("claude 10.0.4\n"), {
|
||||
parts: [10, 0, 4],
|
||||
prerelease: null,
|
||||
});
|
||||
assert.equal(parseClaudeVersion("no version here"), null);
|
||||
});
|
||||
|
||||
test("compareVersions can decide exact equality with the pin", () => {
|
||||
// Both validators require the EXACT pinned version, so what matters is that
|
||||
// "equal" is distinguishable from newer as well as older — a floor would let a
|
||||
// newer local CLI validate against a different schema than CI's.
|
||||
const pin = parseClaudeVersion(PINNED_CLI_VERSION);
|
||||
assert.equal(compareVersions(pin, pin), 0);
|
||||
assert.ok(compareVersions([pin[0], pin[1], pin[2] - 1], pin) < 0);
|
||||
assert.ok(compareVersions([pin[0], pin[1], pin[2] + 1], pin) > 0);
|
||||
assert.ok(compareVersions([pin[0] + 1, 0, 0], pin) > 0);
|
||||
// Shorter and longer triples still order sanely.
|
||||
assert.ok(compareVersions([pin[0]], pin) < 0);
|
||||
assert.equal(compareVersions([...pin, 0], pin), 0);
|
||||
test("parseClaudeVersion keeps a prerelease suffix rather than dropping it", () => {
|
||||
// Dropping it made `2.1.250-beta.1` compare EQUAL to the stable pin — a
|
||||
// different validator schema running while the log claimed an exact match.
|
||||
assert.deepEqual(parseClaudeVersion("2.1.250-beta.1 (Claude Code)"), {
|
||||
parts: [2, 1, 250],
|
||||
prerelease: "beta.1",
|
||||
});
|
||||
assert.equal(
|
||||
formatClaudeVersion(parseClaudeVersion("2.1.250-beta.1")),
|
||||
"2.1.250-beta.1",
|
||||
);
|
||||
assert.equal(formatClaudeVersion(null), "(unreadable version)");
|
||||
});
|
||||
|
||||
test("isPinnedVersion accepts only the exact stable pin", () => {
|
||||
const pin = PINNED_CLI_VERSION;
|
||||
assert.equal(isPinnedVersion(parseClaudeVersion(pin), pin), true);
|
||||
// Build metadata is ignored for precedence by SemVer, so it still matches.
|
||||
assert.equal(
|
||||
isPinnedVersion(parseClaudeVersion(`${pin}+build.7`), pin),
|
||||
true,
|
||||
);
|
||||
// A prerelease of the same triple is NOT the pin.
|
||||
assert.equal(
|
||||
isPinnedVersion(parseClaudeVersion(`${pin}-beta.1`), pin),
|
||||
false,
|
||||
);
|
||||
assert.equal(isPinnedVersion(parseClaudeVersion("2.1.249"), pin), false);
|
||||
assert.equal(isPinnedVersion(parseClaudeVersion("99.0.0"), pin), false);
|
||||
assert.equal(isPinnedVersion(null, pin), false);
|
||||
});
|
||||
|
||||
test("the listing-entry cap covers description AND when_to_use together", () => {
|
||||
|
|
|
|||
|
|
@ -25,14 +25,12 @@ import { fileURLToPath } from "node:url";
|
|||
import { probeClaudeVersion } from "./lib/claude-cli.mjs";
|
||||
import { winShellArgs } from "./lib/win-shell-args.mjs";
|
||||
import {
|
||||
compareVersions,
|
||||
formatClaudeVersion,
|
||||
isPinnedVersion,
|
||||
parseClaudeVersion,
|
||||
PINNED_CLI_VERSION,
|
||||
} from "./lib/skill-manifest.mjs";
|
||||
|
||||
/** `PINNED_CLI_VERSION` as a comparable triple. */
|
||||
const PINNED_VERSION_PARTS = parseClaudeVersion(PINNED_CLI_VERSION) ?? [];
|
||||
|
||||
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
|
||||
const SKILLS_DIR = ".claude/skills";
|
||||
|
||||
|
|
@ -49,20 +47,17 @@ const SKILLS_DIR = ".claude/skills";
|
|||
* @returns {{ command: string, args: string[], via: string }}
|
||||
*/
|
||||
export function validatorCommand({ version }) {
|
||||
if (
|
||||
version !== null &&
|
||||
compareVersions(version, PINNED_VERSION_PARTS) === 0
|
||||
) {
|
||||
if (isPinnedVersion(version, PINNED_CLI_VERSION)) {
|
||||
return {
|
||||
command: "claude",
|
||||
args: ["plugin", "validate", SKILLS_DIR],
|
||||
via: `local claude ${version.join(".")} (matches the pin)`,
|
||||
via: `local claude ${formatClaudeVersion(version)} (matches the pin)`,
|
||||
};
|
||||
}
|
||||
const why =
|
||||
version === null
|
||||
? "no local CLI"
|
||||
: `local CLI is ${version.join(".")}, not the pinned ${PINNED_CLI_VERSION}`;
|
||||
: `local CLI is ${formatClaudeVersion(version)}, not the pinned ${PINNED_CLI_VERSION}`;
|
||||
return {
|
||||
command: "npx",
|
||||
args: [
|
||||
|
|
|
|||
|
|
@ -14,9 +14,13 @@
|
|||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { validatorCommand, runValidator } from "./verify-skills-cli.mjs";
|
||||
import { PINNED_CLI_VERSION } from "./lib/skill-manifest.mjs";
|
||||
import {
|
||||
PINNED_CLI_VERSION,
|
||||
parseClaudeVersion,
|
||||
} from "./lib/skill-manifest.mjs";
|
||||
|
||||
const pinned = PINNED_CLI_VERSION.split(".").map(Number);
|
||||
const pinned = parseClaudeVersion(PINNED_CLI_VERSION);
|
||||
const at = (text) => parseClaudeVersion(text);
|
||||
const npxArgs = [
|
||||
"-y",
|
||||
`@anthropic-ai/claude-code@${PINNED_CLI_VERSION}`,
|
||||
|
|
@ -35,14 +39,18 @@ test("uses a local CLI only when it matches the pin exactly", () => {
|
|||
test("falls back to the pinned package for any other local version", () => {
|
||||
// Newer as well as older: a newer CLI is a DIFFERENT schema from CI's, which
|
||||
// is exactly the cross-machine disagreement the pin exists to prevent.
|
||||
for (const version of [
|
||||
[pinned[0], pinned[1], pinned[2] + 1],
|
||||
[pinned[0], pinned[1], pinned[2] - 1],
|
||||
[pinned[0] + 1, 0, 0],
|
||||
[2, 1, 232],
|
||||
const [maj, min, patch] = pinned.parts;
|
||||
for (const banner of [
|
||||
`${maj}.${min}.${patch + 1}`,
|
||||
`${maj}.${min}.${patch - 1}`,
|
||||
`${maj + 1}.0.0`,
|
||||
"2.1.232",
|
||||
// A prerelease of the pinned triple is a DIFFERENT validator schema, and
|
||||
// dropping the suffix made it compare equal (Copilot).
|
||||
`${maj}.${min}.${patch}-beta.1`,
|
||||
]) {
|
||||
const cmd = validatorCommand({ version });
|
||||
assert.equal(cmd.command, "npx", `expected npx for ${version.join(".")}`);
|
||||
const cmd = validatorCommand({ version: at(banner) });
|
||||
assert.equal(cmd.command, "npx", `expected npx for ${banner}`);
|
||||
assert.deepEqual(cmd.args, npxArgs);
|
||||
assert.match(cmd.via, /not the pinned/);
|
||||
}
|
||||
|
|
@ -72,7 +80,7 @@ function harness({ probe, spawnResult }) {
|
|||
return { code, spawned, logs, errors };
|
||||
}
|
||||
|
||||
const okProbe = PINNED_CLI_VERSION.split(".").map(Number);
|
||||
const okProbe = parseClaudeVersion(PINNED_CLI_VERSION);
|
||||
|
||||
test("runs the local CLI and succeeds when the validator accepts", () => {
|
||||
const r = harness({ probe: okProbe, spawnResult: { status: 0 } });
|
||||
|
|
|
|||
|
|
@ -34,7 +34,8 @@ import { claudeSpawnArgs, probeClaudeVersion } from "./lib/claude-cli.mjs";
|
|||
import { reachableScripts, rootReachesScript } from "./lib/npm-scripts.mjs";
|
||||
import { parse as parseYaml } from "yaml";
|
||||
import {
|
||||
compareVersions,
|
||||
formatClaudeVersion,
|
||||
isPinnedVersion,
|
||||
parseClaudeVersion,
|
||||
PINNED_CLI_VERSION,
|
||||
parseSkill,
|
||||
|
|
@ -46,9 +47,6 @@ import {
|
|||
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
|
||||
const DEFAULT_SKILLS_DIR = path.join(ROOT, ".claude", "skills");
|
||||
|
||||
/** `PINNED_CLI_VERSION` as a comparable triple. */
|
||||
const PINNED_VERSION_PARTS = parseClaudeVersion(PINNED_CLI_VERSION) ?? [];
|
||||
|
||||
/** Directory names under `.claude/skills` that contain a SKILL.md. */
|
||||
export function skillDirs(
|
||||
dir,
|
||||
|
|
@ -273,16 +271,10 @@ function main(argv = process.argv.slice(2)) {
|
|||
// would reintroduce exactly the cross-machine schema drift the pin removes.
|
||||
// Anything else is left to `verify:skills:cli`, which fetches the pin.
|
||||
const version = probeClaudeVersion(parseClaudeVersion);
|
||||
if (version === null) {
|
||||
if (!isPinnedVersion(version, PINNED_CLI_VERSION)) {
|
||||
console.log(
|
||||
"verify:skills — no usable `claude` CLI here; `verify:skills:cli` runs the pinned validator.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (compareVersions(version, PINNED_VERSION_PARTS) !== 0) {
|
||||
console.log(
|
||||
`verify:skills — local \`claude\` is ${version.join(".")}, not the pinned ` +
|
||||
`${PINNED_CLI_VERSION}; \`verify:skills:cli\` runs the pinned validator.`,
|
||||
`verify:skills — local \`claude\` is ${formatClaudeVersion(version)}, not the ` +
|
||||
`pinned ${PINNED_CLI_VERSION}; \`verify:skills:cli\` runs the pinned validator.`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue