diff --git a/.bumpy/surface-bump-file-parse-errors.md b/.bumpy/surface-bump-file-parse-errors.md new file mode 100644 index 0000000..6533c8d --- /dev/null +++ b/.bumpy/surface-bump-file-parse-errors.md @@ -0,0 +1,5 @@ +--- +'@varlock/bumpy': patch +--- + +Surface bump file parse errors to users instead of silently ignoring them diff --git a/packages/bumpy/src/commands/check.ts b/packages/bumpy/src/commands/check.ts index 50ab7d8..d1c077c 100644 --- a/packages/bumpy/src/commands/check.ts +++ b/packages/bumpy/src/commands/check.ts @@ -35,7 +35,13 @@ export async function checkCommand(rootDir: string, opts: CheckOptions = {}): Pr } // Filter to only bump files added/modified on this branch - const allBumpFiles = await readBumpFiles(rootDir); + const { bumpFiles: allBumpFiles, errors: parseErrors } = await readBumpFiles(rootDir); + if (parseErrors.length > 0) { + for (const err of parseErrors) { + log.error(err); + } + process.exit(1); + } const { branchBumpFiles, hasEmptyBumpFile } = filterBranchBumpFiles(allBumpFiles, changedFiles, rootDir); // If an empty bump file exists on this branch, the check passes diff --git a/packages/bumpy/src/commands/ci.ts b/packages/bumpy/src/commands/ci.ts index deba379..cf38248 100644 --- a/packages/bumpy/src/commands/ci.ts +++ b/packages/bumpy/src/commands/ci.ts @@ -94,7 +94,7 @@ export async function ciCheckCommand(rootDir: string, opts: CheckOptions): Promi const config = await loadConfig(rootDir); const { packages } = await discoverWorkspace(rootDir, config); const depGraph = new DependencyGraph(packages); - const allBumpFiles = await readBumpFiles(rootDir); + const { bumpFiles: allBumpFiles, errors: parseErrors } = await readBumpFiles(rootDir); // Skip on the version PR branch — it has no bump files by design const prBranchName = detectPrBranch(rootDir); @@ -110,27 +110,48 @@ export async function ciCheckCommand(rootDir: string, opts: CheckOptions): Promi // Filter to only bump files added/modified in this PR const changedFiles = getChangedFiles(rootDir, config.baseBranch); - const { branchBumpFiles: prBumpFiles, hasEmptyBumpFile } = filterBranchBumpFiles(allBumpFiles, changedFiles, rootDir); + const { branchBumpFiles: prBumpFiles, hasEmptyBumpFile } = filterBranchBumpFiles( + allBumpFiles, + changedFiles, + rootDir, + parseErrors, + ); - // An empty bump file signals intentionally no releases needed - if (hasEmptyBumpFile) { - log.success('Empty bump file found — no releases needed.'); - if (shouldComment && prNumber) { - const prBranch = detectPrBranch(rootDir); - await postOrUpdatePrComment(prNumber, formatNoBumpFilesComment(prBranch, pm), rootDir, opts.patComments); + // Surface any parse errors + if (parseErrors.length > 0) { + for (const err of parseErrors) { + log.error(err); } - return; } if (prBumpFiles.length === 0) { - const willFail = !opts.noFail; - const msg = 'No bump files found in this PR.'; + // An empty bump file signals intentionally no releases needed + if (hasEmptyBumpFile && parseErrors.length === 0) { + log.success('Empty bump file found — no releases needed.'); + if (shouldComment && prNumber) { + await postOrUpdatePrComment(prNumber, formatEmptyBumpFileComment(), rootDir, opts.patComments); + } + return; + } + + const willFail = !opts.noFail || parseErrors.length > 0; + const msg = + parseErrors.length > 0 + ? 'Bump file(s) found but failed to parse — see errors above.' + : 'No bump files found in this PR.'; if (willFail) log.error(msg); else log.warn(msg); if (shouldComment && prNumber) { const prBranch = detectPrBranch(rootDir); - await postOrUpdatePrComment(prNumber, formatNoBumpFilesComment(prBranch, pm), rootDir, opts.patComments); + await postOrUpdatePrComment( + prNumber, + parseErrors.length > 0 + ? formatBumpFileErrorsComment(parseErrors, prBranch, pm) + : formatNoBumpFilesComment(prBranch, pm), + rootDir, + opts.patComments, + ); } if (willFail) process.exit(1); @@ -154,10 +175,15 @@ export async function ciCheckCommand(rootDir: string, opts: CheckOptions): Promi // Comment on PR if (shouldComment && prNumber) { const prBranch = detectPrBranch(rootDir); - const comment = formatReleasePlanComment(plan, prBumpFiles, prNumber, prBranch, pm, plan.warnings); + const comment = formatReleasePlanComment(plan, prBumpFiles, prNumber, prBranch, pm, plan.warnings, parseErrors); await postOrUpdatePrComment(prNumber, comment, rootDir, opts.patComments); } + // Fail if there were parse errors (even if some files parsed successfully) + if (parseErrors.length > 0 && !opts.noFail) { + process.exit(1); + } + // Check for uncovered packages const coveredPackages = new Set(plan.releases.map((r) => r.name)); const changedPackages = await findChangedPackages(changedFiles, packages, rootDir, config); @@ -188,7 +214,14 @@ export async function ciReleaseCommand(rootDir: string, opts: ReleaseOptions): P ensureGitIdentity(rootDir, config); const { packages } = await discoverWorkspace(rootDir, config); const depGraph = new DependencyGraph(packages); - const bumpFiles = await readBumpFiles(rootDir); + const { bumpFiles, errors: releaseParseErrors } = await readBumpFiles(rootDir); + + if (releaseParseErrors.length > 0) { + for (const err of releaseParseErrors) { + log.error(err); + } + throw new Error('Bump file parse errors must be fixed before releasing.'); + } if (bumpFiles.length === 0) { // No bump files — check if there are unpublished packages to publish @@ -440,6 +473,7 @@ function formatReleasePlanComment( prBranch: string | null, pm: PackageManager, warnings: string[] = [], + parseErrors: string[] = [], ): string { const repo = process.env.GITHUB_REPOSITORY; const lines: string[] = []; @@ -488,6 +522,15 @@ function formatReleasePlanComment( } lines.push(''); + if (parseErrors.length > 0) { + lines.push('#### Errors'); + lines.push(''); + for (const e of parseErrors) { + lines.push(`> :x: ${e}`); + } + lines.push(''); + } + if (warnings.length > 0) { lines.push('#### Warnings'); lines.push(''); @@ -509,6 +552,46 @@ function formatReleasePlanComment( return lines.join('\n'); } +function formatBumpFileErrorsComment(errors: string[], prBranch: string | null, pm: PackageManager): string { + const runCmd = pmRunCommand(pm); + const lines = [ + `bumpy-frog`, + '', + '**This PR has bump file(s) with errors that need to be fixed.**', + '
\n', + '#### Errors', + '', + ...errors.map((e) => `> :x: ${e}`), + '', + 'Please fix the errors above or recreate the bump file:\n', + '```bash', + `${runCmd} add`, + '```', + ]; + + const addLink = buildAddBumpFileLink(prBranch); + if (addLink) { + lines.push(''); + lines.push(`Or [click here to add a bump file](${addLink}) directly on GitHub.`); + } + + lines.push('\n---'); + lines.push(`_This comment is maintained by [bumpy](https://bumpy.varlock.dev)._`); + return lines.join('\n'); +} + +function formatEmptyBumpFileComment(): string { + const lines = [ + `bumpy-frog`, + '', + '**This PR includes an empty bump file — no version bump is needed.** :white_check_mark:', + '
', + '\n---', + `_This comment is maintained by [bumpy](https://bumpy.varlock.dev)._`, + ]; + return lines.join('\n'); +} + function formatNoBumpFilesComment(prBranch: string | null, pm: PackageManager): string { const runCmd = pmRunCommand(pm); const lines = [ diff --git a/packages/bumpy/src/commands/status.ts b/packages/bumpy/src/commands/status.ts index 65b2ad7..4089a3d 100644 --- a/packages/bumpy/src/commands/status.ts +++ b/packages/bumpy/src/commands/status.ts @@ -23,7 +23,13 @@ export async function statusCommand(rootDir: string, opts: StatusOptions): Promi const config = await loadConfig(rootDir); const packages = await discoverPackages(rootDir, config); const depGraph = new DependencyGraph(packages); - const bumpFiles = await readBumpFiles(rootDir); + const { bumpFiles, errors: parseErrors } = await readBumpFiles(rootDir); + + if (parseErrors.length > 0) { + for (const err of parseErrors) { + log.error(err); + } + } if (bumpFiles.length === 0) { if (opts.json) { diff --git a/packages/bumpy/src/commands/version.ts b/packages/bumpy/src/commands/version.ts index a52e42d..a938ed5 100644 --- a/packages/bumpy/src/commands/version.ts +++ b/packages/bumpy/src/commands/version.ts @@ -17,7 +17,14 @@ export async function versionCommand(rootDir: string, opts: VersionOptions = {}) const config = await loadConfig(rootDir); const packages = await discoverPackages(rootDir, config); const depGraph = new DependencyGraph(packages); - const bumpFiles = await readBumpFiles(rootDir); + const { bumpFiles, errors: parseErrors } = await readBumpFiles(rootDir); + + if (parseErrors.length > 0) { + for (const err of parseErrors) { + log.error(err); + } + throw new Error('Bump file parse errors must be fixed before versioning.'); + } if (bumpFiles.length === 0) { log.info('No pending bump files.'); diff --git a/packages/bumpy/src/core/bump-file.ts b/packages/bumpy/src/core/bump-file.ts index 4a6949d..0e5bfc2 100644 --- a/packages/bumpy/src/core/bump-file.ts +++ b/packages/bumpy/src/core/bump-file.ts @@ -5,7 +5,6 @@ import { readText, writeText, listFiles, removeFile } from '../utils/fs.ts'; import { getBumpyDir } from './config.ts'; import { tryRunArgs } from '../utils/shell.ts'; import type { BumpFile, BumpFileRelease, BumpFileReleaseCascade, BumpType, BumpTypeWithNone } from '../types.ts'; -import { log } from '../utils/logger.ts'; const VALID_BUMP_TYPES = new Set(['major', 'minor', 'patch', 'none']); @@ -27,15 +26,22 @@ function validatePackageName(name: string): boolean { return true; } +export interface ReadBumpFilesResult { + bumpFiles: BumpFile[]; + errors: string[]; +} + /** Read all bump files from .bumpy/ directory, sorted by git creation order */ -export async function readBumpFiles(rootDir: string): Promise { +export async function readBumpFiles(rootDir: string): Promise { const dir = getBumpyDir(rootDir); const files = await listFiles(dir, '.md'); const bumpFiles: BumpFile[] = []; + const errors: string[] = []; for (const file of files) { if (file === 'README.md') continue; - const bf = await parseBumpFileFromPath(resolve(dir, file)); - if (bf) bumpFiles.push(bf); + const result = await parseBumpFileFromPath(resolve(dir, file)); + if (result.bumpFile) bumpFiles.push(result.bumpFile); + errors.push(...result.errors); } // Sort by the commit date when each bump file was first added to git. @@ -49,7 +55,7 @@ export async function readBumpFiles(rootDir: string): Promise { }); } - return bumpFiles; + return { bumpFiles, errors }; } /** @@ -85,32 +91,57 @@ function getBumpFileCreationOrder(rootDir: string): Map { } /** Parse a single bump file from disk */ -export async function parseBumpFileFromPath(filePath: string): Promise { +export async function parseBumpFileFromPath(filePath: string): Promise { const content = await readText(filePath); return parseBumpFile(content, fileToId(filePath)); } +export interface BumpFileParseResult { + bumpFile: BumpFile | null; + errors: string[]; +} + /** Parse bump file content (for testing) */ -export function parseBumpFile(content: string, id: string): BumpFile | null { - const match = content.match(/^---\n([\s\S]*?)\n---\n?([\s\S]*)$/); - if (!match) return null; +export function parseBumpFile(content: string, id: string): BumpFileParseResult { + const errors: string[] = []; + const match = content.match(/^---\n([\s\S]*?)\n?---\n?([\s\S]*)$/); + if (!match) { + errors.push(`Bump file "${id}" has no valid frontmatter (expected --- delimiters)`); + return { bumpFile: null, errors }; + } const frontmatter = match[1]!; const summary = match[2]!.trim(); - const parsed = yaml.load(frontmatter) as Record; - if (!parsed || typeof parsed !== 'object') return null; + // Empty frontmatter is intentional — signals no releases needed + if (!frontmatter.trim()) { + return { bumpFile: null, errors }; + } + + let parsed: Record; + try { + parsed = yaml.load(frontmatter) as Record; + } catch (e) { + errors.push(`Bump file "${id}" has invalid YAML: ${e instanceof Error ? e.message : e}`); + return { bumpFile: null, errors }; + } + if (!parsed || typeof parsed !== 'object') { + errors.push(`Bump file "${id}" has empty or invalid frontmatter`); + return { bumpFile: null, errors }; + } const releases: BumpFileRelease[] = []; for (const [name, value] of Object.entries(parsed)) { if (!validatePackageName(name)) { - log.warn(`Skipping invalid package name in bump file "${id}": ${name}`); + errors.push(`Invalid package name "${name}" in bump file "${id}"`); continue; } if (typeof value === 'string') { if (!VALID_BUMP_TYPES.has(value)) { - log.warn(`Skipping unknown bump type "${value}" for ${name} in bump file "${id}"`); + errors.push( + `Unknown bump type "${value}" for "${name}" in bump file "${id}" (expected: major, minor, patch, or none)`, + ); continue; } // Simple format: "pkg-name": minor @@ -119,7 +150,9 @@ export function parseBumpFile(content: string, id: string): BumpFile | null { // Nested format: "pkg-name": { bump: minor, cascade: { ... } } const obj = value as { bump: BumpTypeWithNone; cascade?: Record }; if (!VALID_BUMP_TYPES.has(obj.bump)) { - log.warn(`Skipping unknown bump type "${obj.bump}" for ${name} in bump file "${id}"`); + errors.push( + `Unknown bump type "${obj.bump}" for "${name}" in bump file "${id}" (expected: major, minor, patch, or none)`, + ); continue; } const release: BumpFileReleaseCascade = { @@ -128,11 +161,18 @@ export function parseBumpFile(content: string, id: string): BumpFile | null { cascade: obj.cascade || {}, }; releases.push(release); + } else { + errors.push(`Invalid value for "${name}" in bump file "${id}" — expected a bump type string or object`); } } - if (releases.length === 0) return null; - return { id, releases, summary }; + if (releases.length === 0 && errors.length === 0) { + // Truly empty frontmatter with no errors — this is the "intentionally empty" case + return { bumpFile: null, errors }; + } + + const bumpFile = releases.length > 0 ? { id, releases, summary } : null; + return { bumpFile, errors }; } /** Write a bump file */ @@ -190,25 +230,35 @@ export function extractBumpFileIdsFromChangedFiles(changedFiles: string[]): Set< * Filter bump files to only those added/modified on the current branch. * Also detects empty bump files (no releases) that still exist on disk, * which signal intentionally no releases needed. + * + * When `parseErrors` is provided, a file that exists on disk but didn't parse + * is only treated as an "empty bump file" if it produced no parse errors — + * otherwise it's a broken file, not an intentionally empty one. */ export function filterBranchBumpFiles( allBumpFiles: BumpFile[], changedFiles: string[], rootDir?: string, + parseErrors: string[] = [], ): { branchBumpFiles: BumpFile[]; branchBumpFileIds: Set; hasEmptyBumpFile: boolean } { const branchBumpFileIds = extractBumpFileIdsFromChangedFiles(changedFiles); const branchBumpFiles = allBumpFiles.filter((bf) => branchBumpFileIds.has(bf.id)); // Check if any changed bump file IDs that didn't parse still exist on disk (= empty bump file). // Deleted bump files (from other branches) should not count. + // Files that produced parse errors are broken, not intentionally empty. let hasEmptyBumpFile = false; if (rootDir) { const parsedIds = new Set(branchBumpFiles.map((bf) => bf.id)); const bumpyDir = getBumpyDir(rootDir); for (const id of branchBumpFileIds) { if (!parsedIds.has(id) && existsSync(resolve(bumpyDir, `${id}.md`))) { - hasEmptyBumpFile = true; - break; + // Check if this file produced parse errors — if so, it's broken, not empty + const hasErrors = parseErrors.some((e) => e.includes(`"${id}"`)); + if (!hasErrors) { + hasEmptyBumpFile = true; + break; + } } } } diff --git a/packages/bumpy/src/index.ts b/packages/bumpy/src/index.ts index 9245d2e..b74137c 100644 --- a/packages/bumpy/src/index.ts +++ b/packages/bumpy/src/index.ts @@ -3,6 +3,7 @@ export { loadConfig, findRoot, getBumpyDir, matchGlob } from './core/config.ts'; export { discoverPackages } from './core/workspace.ts'; export { DependencyGraph } from './core/dep-graph.ts'; export { readBumpFiles, parseBumpFile, writeBumpFile } from './core/bump-file.ts'; +export type { BumpFileParseResult, ReadBumpFilesResult } from './core/bump-file.ts'; export { assembleReleasePlan } from './core/release-plan.ts'; export { applyReleasePlan } from './core/apply-release-plan.ts'; export { generateChangelogEntry, loadFormatter, defaultFormatter, prependToChangelog } from './core/changelog.ts'; diff --git a/packages/bumpy/test/core/bump-file.test.ts b/packages/bumpy/test/core/bump-file.test.ts index 12f9ee1..d1e6929 100644 --- a/packages/bumpy/test/core/bump-file.test.ts +++ b/packages/bumpy/test/core/bump-file.test.ts @@ -10,7 +10,8 @@ describe('parseBumpFile', () => { Added a new feature to pkg-a `; - const bf = parseBumpFile(content, 'test-bf'); + const { bumpFile: bf, errors } = parseBumpFile(content, 'test-bf'); + expect(errors).toHaveLength(0); expect(bf).not.toBeNull(); expect(bf!.id).toBe('test-bf'); expect(bf!.releases).toHaveLength(2); @@ -29,7 +30,8 @@ Added a new feature to pkg-a Feature in pkg-a, suppress bump on pkg-b `; - const bf = parseBumpFile(content, 'test-bf'); + const { bumpFile: bf, errors } = parseBumpFile(content, 'test-bf'); + expect(errors).toHaveLength(0); expect(bf!.releases).toHaveLength(2); expect(bf!.releases[0]!.type).toBe('minor'); expect(bf!.releases[1]!.type).toBe('none'); @@ -46,7 +48,8 @@ Feature in pkg-a, suppress bump on pkg-b Added encryption provider `; - const bf = parseBumpFile(content, 'test-bf'); + const { bumpFile: bf, errors } = parseBumpFile(content, 'test-bf'); + expect(errors).toHaveLength(0); expect(bf).not.toBeNull(); expect(bf!.releases).toHaveLength(1); const release = bf!.releases[0]! as any; @@ -69,16 +72,67 @@ Added encryption provider Mixed changes `; - const bf = parseBumpFile(content, 'test-bf'); + const { bumpFile: bf, errors } = parseBumpFile(content, 'test-bf'); + expect(errors).toHaveLength(0); expect(bf!.releases).toHaveLength(2); expect(bf!.releases[0]!.name).toBe('@myorg/core'); expect(bf!.releases[1]!.name).toBe('@myorg/utils'); expect(bf!.releases[1]!.type).toBe('patch'); }); - test('returns null for invalid content', () => { - expect(parseBumpFile('no frontmatter here', 'bad')).toBeNull(); - expect(parseBumpFile('---\n---\n', 'empty')).toBeNull(); + test('returns errors for missing frontmatter', () => { + const noFrontmatter = parseBumpFile('no frontmatter here', 'bad'); + expect(noFrontmatter.bumpFile).toBeNull(); + expect(noFrontmatter.errors).toHaveLength(1); + expect(noFrontmatter.errors[0]).toContain('no valid frontmatter'); + }); + + test('returns no errors for intentionally empty bump file (no newline)', () => { + const result = parseBumpFile('---\n---\n', 'empty'); + expect(result.bumpFile).toBeNull(); + expect(result.errors).toHaveLength(0); + }); + + test('returns no errors for intentionally empty bump file (with newline)', () => { + const result = parseBumpFile('---\n\n---\n', 'empty'); + expect(result.bumpFile).toBeNull(); + expect(result.errors).toHaveLength(0); + }); + + test('returns no errors for intentionally empty bump file (with whitespace)', () => { + const result = parseBumpFile('---\n \n---\n', 'empty'); + expect(result.bumpFile).toBeNull(); + expect(result.errors).toHaveLength(0); + }); + + test('returns errors for invalid bump types', () => { + const content = `--- +"pkg-a": bogus +--- + +Bad bump type +`; + const { bumpFile, errors } = parseBumpFile(content, 'test-bf'); + expect(bumpFile).toBeNull(); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('Unknown bump type "bogus"'); + expect(errors[0]).toContain('expected: major, minor, patch, or none'); + }); + + test('returns partial results with errors for mixed valid/invalid entries', () => { + const content = `--- +"pkg-a": minor +"pkg-b": bogus +--- + +Mixed +`; + const { bumpFile, errors } = parseBumpFile(content, 'test-bf'); + expect(bumpFile).not.toBeNull(); + expect(bumpFile!.releases).toHaveLength(1); + expect(bumpFile!.releases[0]!.name).toBe('pkg-a'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('"bogus"'); }); test('handles multi-line summary', () => { @@ -92,7 +146,8 @@ Second paragraph with more details. - bullet point `; - const bf = parseBumpFile(content, 'test-bf'); + const { bumpFile: bf, errors } = parseBumpFile(content, 'test-bf'); + expect(errors).toHaveLength(0); expect(bf!.summary).toContain('First line'); expect(bf!.summary).toContain('Second paragraph'); expect(bf!.summary).toContain('- bullet point');