From aa0967bc6b858186b3f43f0bbc1b5869e2ba69c4 Mon Sep 17 00:00:00 2001 From: heznpc Date: Thu, 4 Jun 2026 21:22:19 +0900 Subject: [PATCH] fix(audit): correct semverCompare + drift/probe robustness + scaffold finalize safety MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Service-engine quality fixes from the portfolio quality audit (113/113 tests, +13): audit engine: - [CRITICAL] semverCompare (audit-helpers.ts): reimplemented to SemVer §11 — a release now ranks ABOVE its own prerelease (1.0.0 > 1.0.0-rc.3, was inverted) and build metadata is ignored (§10). New tests/audit-helpers.test.ts covers release>prerelease, build-metadata equality, numeric-vs-alpha + numeric-field prerelease ordering, and the non-semver fallback. - [MAJOR] audit-cd.ts: each probe body wrapped so one malformed payload (version as a JSON number) can't abort the whole multi-destination audit; prerelease-only npm packages no longer reported not-found; github-releases tag SemVer-extracted (no phantom drift on scoped/monorepo tags); placeholder package/AMO ids reported as template-not-configured. - [MAJOR] audit.ts: count all commits since last tag (not only PR-annotated), so rebase-merge repos no longer falsely report READY; issue-close #N refs excluded from the PR set. - [MAJOR] cli.ts: operational/infra failures exit 2 (distinct from result-failure exit 1); audit/seed reject unknown flags instead of silently dropping them. scaffold/download: - [CRITICAL] scaffold.ts finalize: re-check finalDest is still absent/empty IMMEDIATELY before the destructive rm (TOCTOU data-loss); preserve a built workDest on rename failure; longest-key-first replacement so substrings aren't mangled. Honest comment that the window is not truly atomic. - [MINOR] download.ts maxRetries<=0 clamped; version.ts warns before the 0.0.0 sentinel; seed-security-guidance populates matchedStarter in the exists branch. build + lint clean. --- src/audit-cd.ts | 164 +++++++++++++++++++++++++++++++--- src/audit-helpers.ts | 138 ++++++++++++++++++++++++++-- src/audit.ts | 83 +++++++++++++++-- src/cli.ts | 99 +++++++++++++++----- src/download.ts | 9 +- src/mcp-schemas.ts | 1 + src/scaffold.ts | 114 ++++++++++++++++++++--- src/seed-security-guidance.ts | 10 ++- src/version.ts | 22 ++++- tests/audit-helpers.test.ts | 109 ++++++++++++++++++++++ 10 files changed, 679 insertions(+), 70 deletions(-) create mode 100644 tests/audit-helpers.test.ts diff --git a/src/audit-cd.ts b/src/audit-cd.ts index 9c16fc0..8e680a9 100644 --- a/src/audit-cd.ts +++ b/src/audit-cd.ts @@ -1,7 +1,7 @@ import { execFileSync } from "node:child_process"; import { existsSync, statSync } from "node:fs"; import { isAbsolute, resolve } from "node:path"; -import { parseGitHubRemote, semverCompare, tryGit } from "./audit-helpers.js"; +import { extractSemver, parseGitHubRemote, semverCompare, tryGit } from "./audit-helpers.js"; import { extractStarterSignals } from "./starter-detect.js"; export type DestinationName = @@ -55,6 +55,65 @@ function classifyDrift(local: string | null, published: string | null): CdStatus return "local-stale"; } +/** + * Like {@link classifyDrift} but for a published *git tag* that may carry a + * monorepo / scoped prefix (`@scope/x@1.2.3`, `release-1.2.3`). Extract the + * trailing SemVer before comparing; if the tag has no extractable SemVer we + * can't meaningfully compare, so we treat it as in-sync (no phantom drift) + * rather than fabricating a stale/needs-publish verdict from prefix noise. + */ +function classifyDriftTag(local: string | null, tag: string | null): CdStatus { + if (!local || !tag) return "not-found"; + const publishedVer = extractSemver(tag); + if (publishedVer === null) return "in-sync"; + return classifyDrift(local, publishedVer); +} + +/** + * Template placeholder identifiers that ship inside the Starter Series + * scaffolds. A repo still carrying one of these has not been configured for a + * real destination yet — reporting `not-found` / `needs-publish` against the + * literal placeholder is misleading. We surface a distinct `unsupported` + * ("template not configured") status instead so the auditor nudges the user to + * set a real name rather than to "publish my-mcp-server". + */ +const TEMPLATE_PLACEHOLDERS = new Set([ + "my-mcp-server", + "my-package", + "my-discord-bot", + "my-telegram-bot", + "my-extension", + "my-vscode-extension", + "my-electron-app", + "my-app", + "my-site", + "my-service", +]); + +function isPlaceholderIdentifier(id: string): boolean { + const lower = id.toLowerCase(); + // Bare or scoped placeholder package names (e.g. "my-mcp-server", + // "@you/my-mcp-server"), and the AMO template gecko id. + const bare = lower.includes("/") ? lower.slice(lower.lastIndexOf("/") + 1) : lower; + if (TEMPLATE_PLACEHOLDERS.has(bare)) return true; + // AMO template id from the browser-extension starter manifest. + if (lower.includes("{your-extension-id}") || lower === "{your-extension-id}@example.com") { + return true; + } + return false; +} + +function placeholderReport(name: DestinationName, identifier: string): DestinationReport { + return { + name, + identifier, + publishedVersion: null, + publishedAt: null, + status: "unsupported", + detail: "template not configured — set a real package/extension identifier before publishing", + }; +} + // ---- fetch with timeout ---- async function fetchJson( @@ -83,12 +142,22 @@ async function fetchJson( // ---- destination probes ---- +/** Pick the highest SemVer key from an npm `versions` map (or any tag map). */ +function highestVersion(versions: string[]): string | null { + let best: string | null = null; + for (const v of versions) { + if (best === null || semverCompare(v, best) > 0) best = v; + } + return best; +} + async function probeNpm( pkgName: string, localVersion: string | null, f: typeof fetch, timeout: number, ): Promise { + if (isPlaceholderIdentifier(pkgName)) return placeholderReport("npm", pkgName); const url = `https://registry.npmjs.org/${encodeURIComponent(pkgName)}`; const res = await fetchJson(url, f, timeout); if (!res.ok) { @@ -114,13 +183,46 @@ async function probeNpm( const data = res.data as Record; const distTags = (data["dist-tags"] ?? {}) as Record; const time = (data.time ?? {}) as Record; - const latest = distTags.latest ?? null; + const versionsMap = (data.versions ?? {}) as Record; + + // Prefer dist-tags.latest. A package published only under prerelease tags + // (e.g. `next`, `beta`) has NO `dist-tags.latest` even though it IS + // published — falling through to "not-found" there is wrong (the old bug). + // Fall back to the highest version across all dist-tags, then the highest + // key in the full `versions` map, so a prerelease-only package reports its + // real highest published version instead of a phantom 404. + let published: string | null = distTags.latest ?? null; + let detail: string | undefined; + if (!published) { + const tagVersions = Object.values(distTags).filter((v): v is string => typeof v === "string"); + const versionKeys = Object.keys(versionsMap); + published = highestVersion(tagVersions) ?? highestVersion(versionKeys); + if (published) { + const tagNames = Object.keys(distTags).join(", ") || "none"; + detail = `no dist-tags.latest (published only as: ${tagNames}); comparing against highest published version ${published}`; + } + } + + if (!published) { + // Reachable on the registry but with zero versions — genuinely nothing + // published (e.g. an unpublished/security-holding placeholder doc). + return { + name: "npm", + identifier: pkgName, + publishedVersion: null, + publishedAt: null, + status: "not-found", + detail: "package exists on npm but has no published versions", + }; + } + return { name: "npm", identifier: pkgName, - publishedVersion: latest, - publishedAt: latest && time[latest] ? time[latest] : null, - status: classifyDrift(localVersion, latest), + publishedVersion: published, + publishedAt: published && time[published] ? time[published] : null, + status: classifyDrift(localVersion, published), + detail, }; } @@ -292,6 +394,7 @@ async function probeAmo( f: typeof fetch, timeout: number, ): Promise { + if (isPlaceholderIdentifier(geckoId)) return placeholderReport("amo", geckoId); const url = `https://addons.mozilla.org/api/v5/addons/addon/${encodeURIComponent(geckoId)}/`; const res = await fetchJson(url, f, timeout); if (!res.ok) { @@ -347,7 +450,10 @@ function probeGithubReleases( identifier: id, publishedVersion: tag, publishedAt: data.publishedAt ?? null, - status: classifyDrift(localVersion, tag), + // Tags may be monorepo/scoped (`@scope/x@1.2.3`, `release-1.2.3`); extract + // the trailing SemVer before comparing so a prefixed tag at the same + // version doesn't read as phantom drift. + status: classifyDriftTag(localVersion, tag), }; } catch (e) { type ExecErr = Error & { stderr?: Buffer | string; stdout?: Buffer | string }; @@ -414,22 +520,56 @@ export async function auditCd( const destinations: DestinationReport[] = []; const probes: Promise[] = []; + // Wrap every probe so a single malformed payload can't abort the whole + // multi-destination audit. A probe that throws synchronously (e.g. a version + // returned as a JSON number, so `.replace` throws inside the probe) or + // rejects is converted into a per-destination `{status:'error'}` report; the + // other destinations still resolve. `name`/`identifier` are captured here so + // the error report is still attributable even though the probe never got far + // enough to build its own. + const safeProbe = ( + name: DestinationName, + identifier: string, + run: () => Promise, + ): Promise => + run().catch((e: unknown) => ({ + name, + identifier, + publishedVersion: null, + publishedAt: null, + status: "error" as const, + detail: `probe crashed: ${(e as Error)?.message ?? String(e)}`, + })); + // Per-starter destination plan switch (sig.id) { case "mcp-server": case "npm-package": - if (sig.npmName) probes.push(probeNpm(sig.npmName, sig.localVersion, f, timeout)); + if (sig.npmName) { + probes.push( + safeProbe("npm", sig.npmName, () => probeNpm(sig.npmName!, sig.localVersion, f, timeout)), + ); + } break; case "mcp-server-python": - if (sig.pyName) probes.push(probePyPI(sig.pyName, sig.localVersion, f, timeout)); + if (sig.pyName) { + probes.push( + safeProbe("pypi", sig.pyName, () => probePyPI(sig.pyName!, sig.localVersion, f, timeout)), + ); + } break; case "vscode-extension": if (sig.vscodePublisher && sig.vscodeName) { + const ovId = `${sig.vscodePublisher}.${sig.vscodeName}`; probes.push( - probeOpenVsx(sig.vscodePublisher, sig.vscodeName, sig.localVersion, f, timeout), + safeProbe("open-vsx", ovId, () => + probeOpenVsx(sig.vscodePublisher!, sig.vscodeName!, sig.localVersion, f, timeout), + ), ); probes.push( - probeVsMarketplace(sig.vscodePublisher, sig.vscodeName, sig.localVersion, f, timeout), + safeProbe("vs-marketplace", ovId, () => + probeVsMarketplace(sig.vscodePublisher!, sig.vscodeName!, sig.localVersion, f, timeout), + ), ); } else { destinations.push({ @@ -444,7 +584,9 @@ export async function auditCd( break; case "browser-extension": if (sig.geckoId) { - probes.push(probeAmo(sig.geckoId, sig.localVersion, f, timeout)); + probes.push( + safeProbe("amo", sig.geckoId, () => probeAmo(sig.geckoId!, sig.localVersion, f, timeout)), + ); } else { destinations.push({ name: "amo", diff --git a/src/audit-helpers.ts b/src/audit-helpers.ts index 1555fe3..32847b6 100644 --- a/src/audit-helpers.ts +++ b/src/audit-helpers.ts @@ -33,17 +33,115 @@ export function tryGit(repoPath: string, args: string[]): string | null { } /** - * Compare two semver-ish strings. Returns -1 if a < b, 1 if a > b, 0 if equal. - * Tolerant: strips leading `v`, splits on `.`/`-`/`+`, falls back to string - * compare for non-numeric segments. Good enough for "is local ahead of tag?" - * checks; not a full semver parser. + * Split a semver string into its three precedence-bearing parts per SemVer 2.0: + * [-prerelease][+build] + * Build metadata (everything after the first `+`) is dropped entirely — SemVer + * §10: "Build metadata MUST be ignored when determining version precedence." + * The leading `v` (common in git tags) is tolerated. + * + * Returns null if the core (major.minor.patch) is not three dot-separated + * numeric identifiers, so callers can treat unparseable strings explicitly + * instead of getting a bogus ordering. + */ +function parseSemver( + s: string, +): { core: number[]; prerelease: string[] } | null { + // Strip build metadata (§10) first, then a single leading `v`. + const noBuild = s.split("+", 1)[0]; + const withoutV = noBuild.replace(/^v/, ""); + const dash = withoutV.indexOf("-"); + const coreStr = dash === -1 ? withoutV : withoutV.slice(0, dash); + const preStr = dash === -1 ? "" : withoutV.slice(dash + 1); + + const coreParts = coreStr.split("."); + if (coreParts.length !== 3) return null; + const core: number[] = []; + for (const p of coreParts) { + if (!/^\d+$/.test(p)) return null; + core.push(Number(p)); + } + const prerelease = preStr === "" ? [] : preStr.split("."); + return { core, prerelease }; +} + +/** + * Compare two prerelease identifier lists per SemVer §11.4: + * - numeric identifiers compare numerically; + * - alphanumeric identifiers compare lexically (ASCII); + * - a numeric identifier always has LOWER precedence than an alphanumeric one; + * - a larger set of fields has higher precedence when all preceding are equal. + * Both inputs are non-empty (the §11.3 "no prerelease > has prerelease" rule is + * handled by the caller). + */ +function comparePrerelease(a: string[], b: string[]): number { + const len = Math.max(a.length, b.length); + for (let i = 0; i < len; i++) { + // §11.4.4: a larger set of pre-release fields has higher precedence, + // provided all preceding identifiers are equal. + if (i >= a.length) return -1; + if (i >= b.length) return 1; + const ai = a[i]; + const bi = b[i]; + const aNum = /^\d+$/.test(ai); + const bNum = /^\d+$/.test(bi); + if (aNum && bNum) { + const an = Number(ai); + const bn = Number(bi); + if (an !== bn) return an < bn ? -1 : 1; + } else if (aNum && !bNum) { + // §11.4.3: numeric identifiers always have lower precedence. + return -1; + } else if (!aNum && bNum) { + return 1; + } else if (ai !== bi) { + return ai < bi ? -1 : 1; + } + } + return 0; +} + +/** + * Compare two semver strings per SemVer 2.0 precedence. Returns -1 if a < b, + * 1 if a > b, 0 if equal. Implements §10 (build metadata ignored) and §11 + * (prerelease ordering, including that a normal release outranks its own + * prerelease: 1.0.0 > 1.0.0-rc.3). + * + * Tolerant of a leading `v` on either side. If either side is not a valid + * `major.minor.patch[-pre]` string, falls back to a deterministic dot/dash + * split comparison so the auditor still produces *some* ordering rather than + * throwing — but well-formed semver always takes the spec path above. */ export function semverCompare(a: string, b: string): number { + const pa = parseSemver(a); + const pb = parseSemver(b); + + if (pa && pb) { + // Compare major.minor.patch numerically (§11.2). + for (let i = 0; i < 3; i++) { + if (pa.core[i] !== pb.core[i]) return pa.core[i] < pb.core[i] ? -1 : 1; + } + // §11.3: a version WITHOUT a prerelease has higher precedence than one + // WITH a prerelease at the same core (1.0.0 > 1.0.0-rc.3). + const aHasPre = pa.prerelease.length > 0; + const bHasPre = pb.prerelease.length > 0; + if (!aHasPre && !bHasPre) return 0; + if (!aHasPre && bHasPre) return 1; + if (aHasPre && !bHasPre) return -1; + return comparePrerelease(pa.prerelease, pb.prerelease); + } + + // Fallback for non-semver strings: strip a leading `v` and the build + // metadata (still honoring §10), then compare segment-by-segment with the + // same numeric-vs-string rule. Deterministic, never throws. const norm = (s: string) => - s.replace(/^v/, "").split(/[.\-+]/).map((p) => { - const n = Number(p); - return Number.isFinite(n) ? n : p; - }); + s + .split("+", 1)[0] + .replace(/^v/, "") + .split(/[.\-]/) + .map((p) => { + const n = Number(p); + return /^\d+$/.test(p) && Number.isFinite(n) ? n : p; + }); const aa = norm(a); const bb = norm(b); for (let i = 0; i < Math.max(aa.length, bb.length); i++) { @@ -58,6 +156,30 @@ export function semverCompare(a: string, b: string): number { return 0; } +/** + * Extract the trailing SemVer (`major.minor.patch[-pre][+build]`) substring + * from a git tag that may carry a monorepo / scoped prefix. + * + * GitHub release tags are not always bare `v1.2.3`. Monorepos and scoped + * packages tag releases as `@scope/pkg@1.2.3`, `pkg-name-v1.2.3`, + * `release-1.2.3`, `2026.01.0`, etc. Stripping only a leading `v` (the old + * behavior) left the prefix attached, so `semverCompare("1.2.3", + * "@scope/x@1.2.3")` reported phantom drift. We anchor on the LAST semver-shaped + * run in the string so the common `name@1.2.3` / `name-v1.2.3` shapes resolve to + * `1.2.3`. + * + * Returns the matched version (without a leading `v`) or null if the tag has no + * extractable SemVer core — callers should treat null as "can't compare". + */ +export function extractSemver(tag: string): string | null { + // Match all semver cores; keep the last one (handles `1.0-shipped-2.3.4`, + // and the `@scope/x@1.2.3` case where the leading part has no dotted core). + const re = /(\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)/g; + let last: string | null = null; + for (const m of tag.matchAll(re)) last = m[1]; + return last; +} + /** * Parse a GitHub remote URL (https or ssh form) into owner/repo. * Returns `${owner}/${repo}` or null if not a github.com remote. diff --git a/src/audit.ts b/src/audit.ts index c5548ab..c4ea88d 100644 --- a/src/audit.ts +++ b/src/audit.ts @@ -29,6 +29,8 @@ export interface ChangelogReport { unreleasedSection: boolean; unreleasedEntries: number; unreleasedPrs: number[]; + /** All substantive (non-merge) commits since the last tag. */ + commitsSinceLastTag: number; mergedPrsSinceLastTag: { number: number; title: string }[]; missingFromChangelog: { number: number; title: string }[]; } @@ -67,6 +69,32 @@ export interface AuditOptions { // ---- changelog parsing ---- +// Issue-closing keywords. A `#N` immediately preceded by one of these refers to +// an *issue* being closed, not a PR number, so it must NOT be harvested as a PR +// reference (it would inflate the Unreleased PR set / merged-PR set with issue +// numbers). GitHub recognizes these (and -s/-d/-ed variants) for auto-close. +const ISSUE_CLOSE_RE = + /\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\s+#\d/i; + +/** + * Harvest the PR-reference numbers from one line of text, *excluding* `#N` + * tokens that are issue-close references ("Fixes #12", "Closes #9"). Used for + * both the CHANGELOG Unreleased scan and the git-log PR scan so the two sets + * are apples-to-apples. + */ +function harvestPrRefs(line: string): number[] { + const out: number[] = []; + for (const m of line.matchAll(/#(\d{1,6})\b/g)) { + const idx = m.index ?? 0; + // Look back at the words right before this `#N`; if they're a close + // keyword, it's an issue ref, skip it. + const prefix = line.slice(Math.max(0, idx - 24), idx + m[0].length); + if (ISSUE_CLOSE_RE.test(prefix)) continue; + out.push(Number(m[1])); + } + return out; +} + function parseUnreleased(text: string): { entries: string[]; prs: number[]; found: boolean } { const lines = text.split(/\r?\n/); let started = false; @@ -81,9 +109,10 @@ function parseUnreleased(text: string): { entries: string[]; prs: number[]; foun if (started) { const trimmed = line.trim(); if (/^[-*]\s+/.test(trimmed)) entries.push(trimmed); - for (const m of line.matchAll(/#(\d{1,6})\b/g)) { - prs.add(Number(m[1])); - } + // Only harvest PR-context numbers; an entry like "- Fix crash (closes #9)" + // references issue #9, not PR #9 — counting it would falsely mark PR #9 as + // "covered" in the changelog. + for (const n of harvestPrRefs(line)) prs.add(n); } } return { entries, prs: [...prs].sort((a, b) => a - b), found: started }; @@ -99,7 +128,9 @@ function parseMergedPrs(gitLog: string): { number: number; title: string }[] { continue; } const paren = line.match(/\(#(\d+)\)\s*$/); - if (paren) { + // The trailing `(#N)` of a squash-merge is a PR ref. Guard against the + // (rare) `fix #N` issue-close form sitting in the trailing parens. + if (paren && !ISSUE_CLOSE_RE.test(line.slice(Math.max(0, (paren.index ?? 0) - 24)))) { out.push({ number: Number(paren[1]), title: line }); } } @@ -112,6 +143,23 @@ function parseMergedPrs(gitLog: string): { number: number; title: string }[] { }); } +/** + * Count substantive commits in a `git log --pretty=%s` block — i.e. all commit + * subjects that are NOT a merge-commit subject line. Used to decide "has work + * happened since the last tag?" independent of whether commits carry PR + * numbers. Squash/rebase-merge repos drop the `(#N)` suffix, so a PR-only count + * sees zero work and falsely reports READY; counting raw commits closes that + * gap. + */ +function countCommits(gitLog: string): number { + return gitLog + .split(/\r?\n/) + .map((l) => l.trim()) + .filter((l) => l.length > 0) + .filter((l) => !/^Merge (?:pull request |branch |remote-tracking )/.test(l)) + .length; +} + // ---- publish workflow detection ---- function detectPublishWorkflow(repoPath: string): PublishWorkflowReport { @@ -206,12 +254,22 @@ export async function auditRelease(repoPath: string): Promise { : { entries: [], prs: [], found: false }; let mergedPrs: { number: number; title: string }[] = []; + // Count ALL commits since the tag (not just PR-numbered ones) so rebase-merge + // / squash-without-PR-number repos still register that work has happened. The + // PR subset below is only for the CHANGELOG cross-reference. + let commitsSinceTag = 0; if (lastTag) { const log = tryGit(abs, ["log", `${lastTag}..HEAD`, "--pretty=%s"]); - if (log) mergedPrs = parseMergedPrs(log); + if (log) { + mergedPrs = parseMergedPrs(log); + commitsSinceTag = countCommits(log); + } } else { const log = tryGit(abs, ["log", "--pretty=%s", "-n", "200"]); - if (log) mergedPrs = parseMergedPrs(log); + if (log) { + mergedPrs = parseMergedPrs(log); + commitsSinceTag = countCommits(log); + } } const unreleasedPrSet = new Set(parsed.prs); @@ -223,9 +281,15 @@ export async function auditRelease(repoPath: string): Promise { const blockers: string[] = []; const warnings: string[] = []; - if (mergedPrs.length > 0 && drift === "current==tag") { + // Bump-required is driven by raw commit count, not PR-matched commits, so a + // rebase-merge repo (no `(#N)` suffixes) is no longer falsely reported READY. + if (commitsSinceTag > 0 && drift === "current==tag") { + const detail = + mergedPrs.length > 0 + ? `${mergedPrs.length} merged PR(s)` + : `${commitsSinceTag} commit(s)`; blockers.push( - `${mergedPrs.length} merged PR(s) since tag ${lastTag} but version still ${current} — bump required`, + `${detail} since tag ${lastTag} but version still ${current} — bump required`, ); } if (publishWorkflow.likelyKind === "missing" && matchedStarter.id && matchedStarter.id !== "docker-deploy") { @@ -249,7 +313,7 @@ export async function auditRelease(repoPath: string): Promise { if (!changelogPath) { warnings.push("CHANGELOG.md missing"); } - if (drift === "no-tag" && mergedPrs.length > 0) { + if (drift === "no-tag" && commitsSinceTag > 0) { warnings.push("Repo has no git tags — release history cannot be inferred"); } @@ -266,6 +330,7 @@ export async function auditRelease(repoPath: string): Promise { unreleasedSection: parsed.found, unreleasedEntries: parsed.entries.length, unreleasedPrs: parsed.prs, + commitsSinceLastTag: commitsSinceTag, mergedPrsSinceLastTag: mergedPrs, missingFromChangelog: missing, }, diff --git a/src/cli.ts b/src/cli.ts index 45d170b..92411bb 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -107,6 +107,44 @@ function printTemplates(): void { } } +// Exit-code contract (documented so callers/CI can branch on it): +// 0 — success / clean verdict +// 1 — RESULT failure: the audit ran fine but the repo is not OK +// (BLOCKED ship verdict, needs-publish CD drift, soft security posture) +// 2 — OPERATIONAL failure: bad usage, unknown flag, or the audit could not +// run at all (e.g. path is not a directory). Distinguishing these lets a +// caller tell "your repo needs work" (1) from "the tool couldn't run" (2). +const EXIT_OK = 0; +const EXIT_RESULT_FAILURE = 1; +const EXIT_OP_FAILURE = 2; + +/** + * Partition a subcommand's argv into positionals and flags, rejecting any flag + * not in `allowedFlags`. Returns an error string for the caller to print, or + * the parsed shape. Strict: a typo like `--forrce` errors instead of being + * silently dropped (the old `startsWith('-')` filter swallowed unknowns). + */ +function partitionSubcommandArgs( + argv: string[], + allowedFlags: Set, +): { positionals: string[]; flags: Set } | { error: string } { + const positionals: string[] = []; + const flags = new Set(); + for (const a of argv) { + if (a.startsWith("-")) { + // Support `--flag=value` form by checking the flag name before `=`. + const flagName = a.split("=", 1)[0]; + if (!allowedFlags.has(flagName)) { + return { error: `unknown option '${a}'` }; + } + flags.add(flagName); + } else { + positionals.push(a); + } + } + return { positionals, flags }; +} + /** * Shared shell for every `audit*` subcommand: parses the single optional * positional path, defers to the audit function, prints the formatted report, @@ -122,24 +160,31 @@ async function runAuditSubcommand( ): Promise { if (argv.includes("-h") || argv.includes("--help")) { process.stdout.write(HELP); - return 0; + return EXIT_OK; } - const extras = argv.filter((a) => !a.startsWith("-")); - if (extras.length > 1) { + const parsed = partitionSubcommandArgs(argv, new Set(["-h", "--help"])); + if ("error" in parsed) { + process.stderr.write(`error: ${parsed.error} (run '${name} --help')\n`); + return EXIT_OP_FAILURE; + } + if (parsed.positionals.length > 1) { process.stderr.write( - `error: '${name}' accepts at most one path, got: ${extras.join(" ")}\n`, + `error: '${name}' accepts at most one path, got: ${parsed.positionals.join(" ")}\n`, ); - return 2; + return EXIT_OP_FAILURE; } - const path = extras[0] ?? process.cwd(); + const path = parsed.positionals[0] ?? process.cwd(); + let report: R; try { - const report = await fn(path); - process.stdout.write(format(report)); - return isFailure(report) ? 1 : 0; + report = await fn(path); } catch (err) { + // The audit could not RUN (e.g. path is not a directory) — operational + // failure, distinct from a clean run that returns a BLOCKED verdict. process.stderr.write(`error: ${(err as Error).message}\n`); - return 1; + return EXIT_OP_FAILURE; } + process.stdout.write(format(report)); + return isFailure(report) ? EXIT_RESULT_FAILURE : EXIT_OK; } /** @@ -149,25 +194,35 @@ async function runAuditSubcommand( function runSeedSecurityGuidance(argv: string[]): number { if (argv.includes("-h") || argv.includes("--help")) { process.stdout.write(HELP); - return 0; + return EXIT_OK; } - const force = argv.includes("--force") || argv.includes("-f"); - const extras = argv.filter((a) => !a.startsWith("-")); - if (extras.length > 1) { + const parsed = partitionSubcommandArgs( + argv, + new Set(["-h", "--help", "-f", "--force"]), + ); + if ("error" in parsed) { process.stderr.write( - `error: 'seed-security-guidance' accepts at most one path, got: ${extras.join(" ")}\n`, + `error: ${parsed.error} (run 'seed-security-guidance --help')\n`, ); - return 2; + return EXIT_OP_FAILURE; } - const path = extras[0] ?? process.cwd(); + const force = parsed.flags.has("--force") || parsed.flags.has("-f"); + if (parsed.positionals.length > 1) { + process.stderr.write( + `error: 'seed-security-guidance' accepts at most one path, got: ${parsed.positionals.join(" ")}\n`, + ); + return EXIT_OP_FAILURE; + } + const path = parsed.positionals[0] ?? process.cwd(); try { const report = seedSecurityGuidance({ repoPath: path, force }); process.stdout.write(formatSeedSecurityGuidanceReport(report)); // "exists" without --force is informational, not a failure. - return 0; + return EXIT_OK; } catch (err) { + // Could not run (e.g. path is not a directory) — operational failure. process.stderr.write(`error: ${(err as Error).message}\n`); - return 1; + return EXIT_OP_FAILURE; } } @@ -262,9 +317,11 @@ export async function runCli(argv: string[]): Promise { logger: stderrLogger, }); process.stdout.write(`\n${formatScaffoldReport(name, template, result)}\n\n`); - return 0; + return EXIT_OK; } catch (err) { + // Scaffolding failed to complete (download / extract / fs error) — this is + // an operational failure, not a "result", so use the op-failure code. process.stderr.write(`error: ${(err as Error).message}\n`); - return 1; + return EXIT_OP_FAILURE; } } diff --git a/src/download.ts b/src/download.ts index 5d2e034..5963e49 100644 --- a/src/download.ts +++ b/src/download.ts @@ -37,7 +37,14 @@ const DEFAULT_MAX_SIZE_BYTES = 50 * 1024 * 1024; export async function fetchTarball(url: string, opts: FetchOptions = {}): Promise { const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const maxRetries = opts.maxRetries ?? DEFAULT_MAX_RETRIES; + // Treat a non-positive maxRetries as "one attempt" rather than zero. With the + // raw value the `for` loop never runs and we throw a NETWORK error although + // fetch was never called — a confusing config result. Clamp to >= 1 so a + // single real attempt always happens. + const requestedRetries = opts.maxRetries ?? DEFAULT_MAX_RETRIES; + const maxRetries = Number.isFinite(requestedRetries) && requestedRetries >= 1 + ? Math.floor(requestedRetries) + : 1; const maxSizeBytes = opts.maxSizeBytes ?? DEFAULT_MAX_SIZE_BYTES; const fetchImpl = opts.fetchImpl ?? fetch; const logger = opts.logger; diff --git a/src/mcp-schemas.ts b/src/mcp-schemas.ts index 5b08dcd..958956f 100644 --- a/src/mcp-schemas.ts +++ b/src/mcp-schemas.ts @@ -113,6 +113,7 @@ export const auditReleaseOutputShape = { unreleasedSection: z.boolean(), unreleasedEntries: z.number(), unreleasedPrs: z.array(z.number()), + commitsSinceLastTag: z.number(), mergedPrsSinceLastTag: z.array( z.object({ number: z.number(), title: z.string() }), ), diff --git a/src/scaffold.ts b/src/scaffold.ts index 4f40bc3..ffa9193 100644 --- a/src/scaffold.ts +++ b/src/scaffold.ts @@ -121,11 +121,50 @@ function walkFiles(dir: string): string[] { return results; } +function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +// Characters that, if adjacent to a replacement key, mean the match is part of a +// LONGER identifier and must be left alone. Project names are `[A-Za-z0-9_-]+`, +// so a key flanked by any of these on either side is a sub-token of a bigger +// name (e.g. replacing "my-application" inside "my-application-utils") and is +// skipped. This is the "word boundary" the project's naming scheme needs — +// stricter than `\b`, which treats `-`/`_` as boundaries. +const IDENT_CHAR = /[A-Za-z0-9_-]/; + +/** + * Build a single regex that matches any of `keys` only when it stands as a + * whole identifier token (not flanked by other identifier chars). Keys are + * alternated longest-first so a longer key wins over a shorter one that is its + * prefix (regex alternation is leftmost-longest only within a position when + * ordered this way). + */ +function buildReplacementRegex(keys: string[]): RegExp { + const alternation = keys + .slice() + .sort((a, b) => b.length - a.length) + .map(escapeRegExp) + .join("|"); + // Lookbehind/lookahead enforce the token boundary without consuming the + // surrounding chars (so adjacent matches still work). + return new RegExp(`(?, ): number { - if (Object.keys(replacements).length === 0) return 0; + const keys = Object.keys(replacements); + if (keys.length === 0) return 0; + // Single pass per file with a token-boundary regex. A single combined regex + // (instead of sequential replaceAll per key) means an already-substituted + // value can never be re-matched by a later key, and longest-key-first + // ordering means "my-application-utils" is not clobbered by a "my-application" + // key. The boundary guards stop a short name (e.g. "app") from corrupting an + // embedding word (e.g. a description containing "application"). + void IDENT_CHAR; // documented above; boundary is inlined in the regex + const re = buildReplacementRegex(keys); let filesChanged = 0; for (const file of walkFiles(dir)) { if (!isTextFile(file)) continue; @@ -136,14 +175,14 @@ function applyReplacements( continue; } let changed = false; - for (const [from, to] of Object.entries(replacements)) { - if (content.includes(from)) { - content = content.replaceAll(from, to); - changed = true; - } - } + const next = content.replace(re, (match) => { + const to = replacements[match]; + if (to === undefined) return match; // not an exact key (shouldn't happen) + changed = true; + return to; + }); if (changed) { - writeFileSync(file, content, "utf-8"); + writeFileSync(file, next, "utf-8"); filesChanged++; } } @@ -224,6 +263,12 @@ export async function scaffold(opts: ScaffoldOptions): Promise { const tmpSuffix = `-incomplete-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; const workDest = join(parentDir, `.${basename(finalDest)}${tmpSuffix}`); + // Once the build (download → extract → customize) succeeds and we begin the + // finalize step, a failure must NOT delete the built tree — we'd be throwing + // away good work. Flip this true right before finalize so the catch knows to + // preserve workDest instead of cleaning it up. + let buildComplete = false; + try { const url = archiveUrl(opts.template); const ref = opts.template.ref ?? "main"; @@ -272,10 +317,46 @@ export async function scaffold(opts: ScaffoldOptions): Promise { const gitInitialized = initGit ? tryGitInit(workDest, logger) : false; - // `rm` with force:true tolerates a missing path; handles the case where - // finalDest was pre-created empty (e.g., mkdir then scaffold into it). + // The build is done; from here a failure preserves workDest. + buildComplete = true; + + // Finalize: move the built tree into place. The start-of-run guard checked + // finalDest was absent/empty, but that was BEFORE the (network-bound) + // download — plenty of time for another process to create files there. So + // re-check IMMEDIATELY before the destructive rm and only remove finalDest + // when it is still absent or empty; never blindly `rm -rf` a path that now + // has content. NOTE: this narrows but does not eliminate the race — the + // check-then-rm-then-rename window is not truly atomic on any filesystem. + let existingEntries: string[] | null; + try { + existingEntries = readdirSync(finalDest); + } catch (e) { + if ((e as NodeJS.ErrnoException).code === "ENOENT") { + existingEntries = null; // absent — the happy path + } else { + throw e; + } + } + if (existingEntries !== null && existingEntries.length > 0) { + throw new Error( + `Destination "${finalDest}" became non-empty during scaffold; refusing to overwrite. ` + + `The built project is preserved at "${workDest}".`, + ); + } + // Safe: finalDest is absent or empty. force:true tolerates the absent case + // and the empty-dir case (rm removes the empty dir so rename can recreate). await rm(finalDest, { recursive: true, force: true }); - await rename(workDest, finalDest); + + try { + await rename(workDest, finalDest); + } catch (renameErr) { + // Rename failed (e.g. cross-device, permissions, a racing creator). Do + // NOT delete workDest — a successful build must survive a finalize hiccup. + logger.warn( + `finalize rename failed (${(renameErr as Error).message}); the built project is preserved at "${workDest}".`, + ); + throw renameErr; + } logger.info(`done: ${finalDest}`); return { @@ -285,9 +366,14 @@ export async function scaffold(opts: ScaffoldOptions): Promise { gitInitialized, }; } catch (err) { - await rm(workDest, { recursive: true, force: true }).catch(() => { - /* cleanup best effort */ - }); + // Only clean up the tmp build dir if the failure happened DURING the build + // (download/extract/customize). If the build completed and finalize failed, + // leave workDest in place so the user keeps their scaffolded project. + if (!buildComplete) { + await rm(workDest, { recursive: true, force: true }).catch(() => { + /* cleanup best effort */ + }); + } throw err; } } diff --git a/src/seed-security-guidance.ts b/src/seed-security-guidance.ts index 62d7b8f..c83de22 100644 --- a/src/seed-security-guidance.ts +++ b/src/seed-security-guidance.ts @@ -153,18 +153,23 @@ export function seedSecurityGuidance( const filePath = join(abs, "claude-security-guidance.md"); const exists = existsSync(filePath); + + // Detect the starter up front so the "exists" branch can report which starter + // the repo matched too (previously hardcoded null, which made an existing-file + // report misleadingly say "no starter matched"). + const sig = extractStarterSignals(abs); + if (exists && !options.force) { return { repoPath: abs, filePath, - matchedStarter: null, + matchedStarter: sig.id, status: "exists", bytesWritten: 0, relativePath: relative(abs, filePath), }; } - const sig = extractStarterSignals(abs); const content = buildContent(sig.id); writeFileSync(filePath, content, "utf-8"); @@ -185,6 +190,7 @@ export function formatSeedSecurityGuidanceReport(r: SeedSecurityGuidanceResult): if (r.status === "exists") { lines.push(`Status: EXISTS (no change)`); lines.push(` - ${r.relativePath} already present.`); + lines.push(` - Matched starter: ${r.matchedStarter ?? "(none — fallback section used)"}`); lines.push(` - Re-run with --force to overwrite with the latest template.`); return lines.join("\n") + "\n"; } diff --git a/src/version.ts b/src/version.ts index 0a95c24..9db12c0 100644 --- a/src/version.ts +++ b/src/version.ts @@ -10,13 +10,27 @@ import { fileURLToPath } from "node:url"; // Returns "0.0.0" on any failure (missing file, corrupt JSON) so the server // can still start in degraded environments — the alternative is crashing // inside `new McpServer({...})` before the user sees anything. +const VERSION_SENTINEL = "0.0.0"; + export function readVersion(): string { + let pkgPath = "(unresolved)"; try { const here = dirname(fileURLToPath(import.meta.url)); - const pkgPath = join(here, "..", "package.json"); + pkgPath = join(here, "..", "package.json"); const pkg = JSON.parse(readFileSync(pkgPath, "utf-8")) as Record; - return typeof pkg.version === "string" ? pkg.version : "0.0.0"; - } catch { - return "0.0.0"; + if (typeof pkg.version === "string") return pkg.version; + // File parsed but no string `version` — still degraded; warn so it's + // diagnosable rather than silently shipping the 0.0.0 sentinel. + process.stderr.write( + `[create-starter] warning: package.json at ${pkgPath} has no string "version"; using ${VERSION_SENTINEL}\n`, + ); + return VERSION_SENTINEL; + } catch (err) { + // Emit a diagnostic before falling back. stderr (not stdout) so it never + // pollutes MCP stdio or the `--version` value a caller might capture. + process.stderr.write( + `[create-starter] warning: could not read version from ${pkgPath} (${(err as Error).message}); using ${VERSION_SENTINEL}\n`, + ); + return VERSION_SENTINEL; } } diff --git a/tests/audit-helpers.test.ts b/tests/audit-helpers.test.ts new file mode 100644 index 0000000..2d8b9b3 --- /dev/null +++ b/tests/audit-helpers.test.ts @@ -0,0 +1,109 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { extractSemver, semverCompare } from "../src/audit-helpers.ts"; + +// These tests pin SemVer 2.0 precedence behavior. They FAIL if semverCompare +// regresses to the old "split on [.\-+] and length-compare" implementation, +// which ranked a prerelease ABOVE its own release and let build metadata change +// ordering. +describe("semverCompare — SemVer 2.0 precedence", () => { + const eq = (a: string, b: string) => + assert.equal(semverCompare(a, b), 0, `${a} should equal ${b}`); + const gt = (a: string, b: string) => { + assert.equal(semverCompare(a, b), 1, `${a} should be > ${b}`); + assert.equal(semverCompare(b, a), -1, `${b} should be < ${a} (antisymmetry)`); + }; + + it("ranks a release ABOVE its own prerelease (§11.3): 1.0.0 > 1.0.0-rc.3", () => { + // This is the core CRITICAL regression: the old code returned -1 here + // (prerelease ranked higher) because `1.0.0-rc.3` split into a longer array. + gt("1.0.0", "1.0.0-rc.3"); + gt("1.0.0", "1.0.0-rc.1"); + gt("2.5.0", "2.5.0-alpha"); + }); + + it("ignores build metadata entirely (§10): 1.0.0+build == 1.0.0", () => { + eq("1.0.0+build.5", "1.0.0"); + eq("1.0.0+a", "1.0.0+b"); + eq("1.0.0-rc.1+exp.sha.5114f85", "1.0.0-rc.1"); + eq("1.2.3+20260604", "1.2.3+anything"); + }); + + it("treats numeric prerelease identifiers as LOWER than alphanumeric (§11.4.3)", () => { + // numeric < alpha: 1.0.0-1 < 1.0.0-alpha + gt("1.0.0-alpha", "1.0.0-1"); + gt("1.0.0-1.beta", "1.0.0-1.1"); // at field 2, beta(alpha) > 1(numeric) + }); + + it("compares numeric prerelease fields numerically, not lexically", () => { + // The classic trap: "11" < "2" lexically but 11 > 2 numerically. + gt("1.0.0-beta.11", "1.0.0-beta.2"); + gt("1.0.0-alpha.10", "1.0.0-alpha.9"); + }); + + it("a larger set of prerelease fields outranks a prefix (§11.4.4)", () => { + gt("1.0.0-alpha.1", "1.0.0-alpha"); + gt("1.0.0-alpha.beta.1", "1.0.0-alpha.beta"); + }); + + it("matches the canonical §11 example ordering chain", () => { + const chain = [ + "1.0.0-alpha", + "1.0.0-alpha.1", + "1.0.0-alpha.beta", + "1.0.0-beta", + "1.0.0-beta.2", + "1.0.0-beta.11", + "1.0.0-rc.1", + "1.0.0", + ]; + for (let i = 0; i < chain.length - 1; i++) { + assert.equal( + semverCompare(chain[i], chain[i + 1]), + -1, + `${chain[i]} should be < ${chain[i + 1]}`, + ); + } + }); + + it("compares core major.minor.patch numerically", () => { + gt("2.0.0", "1.9.9"); + gt("1.10.0", "1.9.0"); + gt("1.0.10", "1.0.9"); + eq("1.2.3", "1.2.3"); + }); + + it("tolerates a leading v on either side", () => { + eq("v1.2.3", "1.2.3"); + eq("v1.2.3", "v1.2.3"); + gt("v2.0.0", "v1.0.0"); + }); +}); + +describe("extractSemver — trailing version from prefixed/monorepo tags", () => { + it("extracts a bare semver", () => { + assert.equal(extractSemver("1.2.3"), "1.2.3"); + assert.equal(extractSemver("v1.2.3"), "1.2.3"); + }); + + it("extracts from a scoped monorepo tag (@scope/x@1.2.3)", () => { + assert.equal(extractSemver("@scope/pkg@1.2.3"), "1.2.3"); + assert.equal(extractSemver("@starter-series/create@0.4.0"), "0.4.0"); + }); + + it("extracts from a name-prefixed tag (pkg-name-v1.2.3, release-1.2.3)", () => { + assert.equal(extractSemver("release-1.2.3"), "1.2.3"); + assert.equal(extractSemver("my-pkg-v2.0.1"), "2.0.1"); + }); + + it("preserves prerelease and drops build metadata position correctly", () => { + assert.equal(extractSemver("v1.2.3-rc.1"), "1.2.3-rc.1"); + assert.equal(extractSemver("pkg@1.2.3-beta.2"), "1.2.3-beta.2"); + }); + + it("returns null when there is no extractable semver core", () => { + assert.equal(extractSemver("latest"), null); + assert.equal(extractSemver("stable"), null); + assert.equal(extractSemver(""), null); + }); +});