From 57ffd8373121f8f0de6d874ff661d4c1e9c06153 Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 28 Aug 2026 01:08:54 +0900 Subject: [PATCH 1/5] feat(cli): collect config validation issues as diagnostics in loadChecklyConfig [RED-914] Convert the four throw-based, fail-fast config validators to report through the Diagnostics system instead. All config problems are now collected in a single run into a ConfigFileDiagnostics collector that attributes each diagnostic to the config file. When any fatal diagnostic is present, loading fails with a new InvalidConfigError carrying the diagnostics; otherwise the collector is returned alongside the config for later rendering. Co-Authored-By: Claude Fable 5 --- .../__tests__/checkly-config-loader.spec.ts | 157 ++++++++++++------ .../__tests__/config-diagnostics.spec.ts | 78 +++++++++ .../configs/embedded-packages-multiple-bad.js | 14 ++ .../fixtures/configs/multiple-errors.js | 13 ++ .../fixtures/configs/non-string-fields.js | 9 + .../cli/src/services/checkly-config-loader.ts | 111 ++++++++++--- .../cli/src/services/config-diagnostics.ts | 66 ++++++++ 7 files changed, 372 insertions(+), 76 deletions(-) create mode 100644 packages/cli/src/services/__tests__/config-diagnostics.spec.ts create mode 100644 packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js create mode 100644 packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js create mode 100644 packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js create mode 100644 packages/cli/src/services/config-diagnostics.ts diff --git a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts index ac279427..d9af5fef 100644 --- a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts +++ b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts @@ -3,27 +3,69 @@ import path from 'node:path' import { describe, it, expect } from 'vitest' import { loadChecklyConfig, defaultFilenames } from '../checkly-config-loader.js' +import { InvalidConfigError } from '../config-diagnostics.js' import { splitConfigFilePath } from '../util.js' +const configDir = path.join(__dirname, 'fixtures', 'configs') + +async function loadInvalidConfig (filename: string): Promise { + const error = await loadChecklyConfig(configDir, [filename]).then( + () => undefined, + err => err, + ) + expect(error).toBeInstanceOf(InvalidConfigError) + return error +} + describe('loadChecklyConfig()', () => { it('config file should export an object', async () => { - try { - await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), ['no-export-config.js']) - } catch (e: any) { - expect(e.message).toContain('Config object missing a logicalId as type string') - } + await expect(loadChecklyConfig(configDir, ['no-export-config.js'])) + .rejects.toThrow(`Property "logicalId" is required and must be set`) }) it('config file should export an object with projectName and logicalId', async () => { - try { - await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), ['no-logical-id-config.js']) - } catch (e: any) { - expect(e.message).toContain('Config object missing a logicalId as type string') - } + await expect(loadChecklyConfig(configDir, ['no-logical-id-config.js'])) + .rejects.toThrow(`Property "logicalId" is required and must be set`) + }) + it('reports all config errors in a single run', async () => { + const error = await loadInvalidConfig('multiple-errors.js') + expect(error.diagnostics.isFatal()).toBe(true) + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`Property "logicalId" is required and must be set`), + }), + expect.objectContaining({ + message: expect.stringContaining(`The value provided for property "bundle" is not valid`), + }), + expect.objectContaining({ + message: expect.stringContaining(`Property "runner.registires" is not supported`), + }), + ])) + }) + it('attributes diagnostics to the config file', async () => { + const error = await loadInvalidConfig('multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringMatching(/^\[\S*multiple-errors\.js\] /), + }), + ])) + }) + it('reports present but non-string required fields as invalid values', async () => { + const error = await loadInvalidConfig('non-string-fields.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining(`Invalid property value`), + message: expect.stringContaining(`The value provided for property "logicalId" is not valid`), + }), + expect.objectContaining({ + title: expect.stringContaining(`Invalid property value`), + message: expect.stringContaining(`The value provided for property "projectName" is not valid`), + }), + ])) }) it('error should indicate the tried file name combinations', async () => { - const configDir = path.join(__dirname, 'fixtures', 'not-existing-config-path') + const missingConfigDir = path.join(__dirname, 'fixtures', 'not-existing-config-path') try { - await loadChecklyConfig(configDir) + await loadChecklyConfig(missingConfigDir) } catch (e: any) { expect(e.message).toContain(`Unable to detect a Checkly configuration file`) for (const filename of defaultFilenames) { @@ -41,7 +83,8 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + diagnostics, + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { @@ -51,6 +94,8 @@ describe('loadChecklyConfig()', () => { }, }, }) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toEqual([]) }) it('config JS file should export an object', async () => { const filename = 'good-config.js' @@ -62,7 +107,7 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { @@ -75,39 +120,39 @@ describe('loadChecklyConfig()', () => { }) it('accepts a string caching.dependencyCache.version', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-string.ts'], ) expect(config.caching?.dependencyCache?.version).toBe('v2') }) it('accepts 0 as a caching.dependencyCache.version', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-zero.ts'], ) expect(config.caching?.dependencyCache?.version).toBe(0) }) it('rejects a non-integer caching.dependencyCache.version', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-float.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`The value provided for property "caching.dependencyCache.version" is not valid`) }) it('rejects an unsafe integer caching.dependencyCache.version', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-unsafe-integer.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`must be a safe integer if given as a number`) }) it('rejects a caching.dependencyCache.version that is neither string nor number', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['dependency-cache-version-bad-type.js'], - )).rejects.toThrow(`Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`) + )).rejects.toThrow(`must be a string or a safe integer`) }) it('accepts valid bundle.packages.embed entries', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-valid.ts'], ) expect(config.bundle?.packages?.embed) @@ -115,44 +160,55 @@ describe('loadChecklyConfig()', () => { }) it('rejects a bundle.packages.embed that is not an array', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-not-array.js'], - )).rejects.toThrow(`Config field 'bundle.packages.embed' must be an array of strings if set`) + )).rejects.toThrow(`The value provided for property "bundle.packages.embed" is not valid`) }) it('rejects a bundle that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-bundle-not-object.js'], - )).rejects.toThrow(`Config field 'bundle' must be an object if set`) + )).rejects.toThrow(`The value provided for property "bundle" is not valid`) }) it('rejects a bundle.packages that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-packages-not-object.js'], - )).rejects.toThrow(`Config field 'bundle.packages' must be an object if set`) + )).rejects.toThrow(`The value provided for property "bundle.packages" is not valid`) }) it('rejects a bundle.packages.embed entry that is not a valid package name', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-bad-name.js'], )).rejects.toThrow(`is not a valid npm package name`) }) it('rejects a bundle.packages.embed entry with a version range', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['embedded-packages-range-version.js'], )).rejects.toThrow(`is not an exact semver version`) }) + it('reports every invalid bundle.packages.embed entry', async () => { + const error = await loadInvalidConfig('embedded-packages-multiple-bad.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`is not a valid npm package name`), + }), + expect.objectContaining({ + message: expect.stringContaining(`is not an exact semver version`), + }), + ])) + }) it('accepts a bundle.packages.prune pattern array', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-valid.ts'], ) expect(config.bundle?.packages?.prune).toEqual(['@acme/*', '!@acme/keep', 'left-pad']) }) it('accepts a bundle.packages.prune per-class map', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-valid-classes.ts'], ) expect(config.bundle?.packages?.prune).toEqual({ @@ -162,37 +218,35 @@ describe('loadChecklyConfig()', () => { }) it('rejects a bundle.packages.prune that is neither array nor object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-shape.js'], )).rejects.toThrow( - `Config field 'bundle.packages.prune' is invalid: must be an array of package name patterns` - + ` or an object keyed by dependency class`, + `must be an array of package name patterns or an object keyed by dependency class`, ) }) it('rejects a bundle.packages.prune with an unknown dependency class', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-class.js'], - )).rejects.toThrow(`Config field 'bundle.packages.prune' is invalid: 'peerDependences' is not a dependency class`) + )).rejects.toThrow(`'peerDependences' is not a dependency class`) }) it('rejects a bundle.packages.prune class value that is neither true nor an array', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-class-value.js'], )).rejects.toThrow( - `Config field 'bundle.packages.prune' is invalid: 'peerDependencies' must be true` - + ` or an array of package name patterns`, + `'peerDependencies' must be true or an array of package name patterns`, ) }) it('rejects a bundle.packages.prune entry with an embed-style version pin', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['bundle-packages-prune-bad-pattern.js'], )).rejects.toThrow(`'name@version' pins are not supported here`) }) it('accepts a valid runner.registries configuration', async () => { const { config } = await loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-valid.ts'], ) expect(config.runner?.registries).toEqual({ @@ -211,28 +265,27 @@ describe('loadChecklyConfig()', () => { }) it('rejects a runner that is not an object', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-runner-not-object.js'], - )).rejects.toThrow(`Config field 'runner' must be an object if set`) + )).rejects.toThrow(`The value provided for property "runner" is not valid`) }) it('rejects a misspelled key inside the runner block', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-misspelled-key.js'], - )).rejects.toThrow(`Config field 'runner' contains unknown field 'registires' (expected only: 'registries')`) + )).rejects.toThrow(`Property "runner.registires" is not supported`) }) it('rejects a runner.registries rule using an unknown upstream name', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-unknown-upstream.js'], )).rejects.toThrow( - `Config field 'runner.registries' is invalid: packages[0]: upstream 'mirror' is not defined` - + ` under 'upstreams' (defined: 'npmjs')`, + `packages[0]: upstream 'mirror' is not defined under 'upstreams' (defined: 'npmjs')`, ) }) it('rejects a runner.registries auth token without a ${VAR} reference', async () => { await expect(loadChecklyConfig( - path.join(__dirname, 'fixtures', 'configs'), + configDir, ['runner-registries-literal-token.js'], )).rejects.toThrow(/must be exactly one environment variable reference in \$\{VAR\} syntax/) }) @@ -246,7 +299,7 @@ describe('loadChecklyConfig()', () => { const { config, - } = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename]) + } = await loadChecklyConfig(configDir, [filename]) expect(config).toMatchObject({ checks: { diff --git a/packages/cli/src/services/__tests__/config-diagnostics.spec.ts b/packages/cli/src/services/__tests__/config-diagnostics.spec.ts new file mode 100644 index 00000000..a715f9ff --- /dev/null +++ b/packages/cli/src/services/__tests__/config-diagnostics.spec.ts @@ -0,0 +1,78 @@ +import { describe, it, expect } from 'vitest' + +import { ConfigFileDiagnostic, ConfigFileDiagnostics, InvalidConfigError } from '../config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' + +describe('ConfigFileDiagnostic', () => { + it('prefixes the title with the config file name and delegates severity', () => { + const underlying = new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value is not valid.', + error: new Error('The value is not valid.'), + }) + const diagnostic = new ConfigFileDiagnostic('checkly.config.ts', underlying) + expect(diagnostic.title).toBe('[checkly.config.ts] Invalid property value') + expect(diagnostic.message).toBe('The value is not valid.') + expect(diagnostic.isFatal()).toBe(true) + expect(diagnostic.isBenign()).toBe(false) + }) +}) + +describe('ConfigFileDiagnostics', () => { + it('wraps added diagnostics in ConfigFileDiagnostic', () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new WarningDiagnostic({ + title: 'Some warning', + message: 'Something looks off.', + })) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toEqual([ + expect.objectContaining({ + title: '[checkly.config.ts] Some warning', + }), + ]) + }) + + it('renders before later diagnostics when extended into a collector first', () => { + const configDiagnostics = new ConfigFileDiagnostics('checkly.config.ts') + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + + const diagnostics = new Diagnostics() + diagnostics.extend(configDiagnostics) + diagnostics.add(new WarningDiagnostic({ + title: 'Construct warning', + message: 'A construct-level warning.', + })) + + expect(diagnostics.observations.map(diagnostic => diagnostic.title)).toEqual([ + '[checkly.config.ts] Config warning', + 'Construct warning', + ]) + }) +}) + +describe('InvalidConfigError', () => { + it('composes its message from the fatal observations', () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value provided for property "bundle" is not valid.', + error: new Error('The value provided for property "bundle" is not valid.'), + })) + diagnostics.add(new WarningDiagnostic({ + title: 'Some warning', + message: 'Not included in the message.', + })) + + const error = new InvalidConfigError(diagnostics) + expect(error.name).toBe('InvalidConfigError') + expect(error.diagnostics).toBe(diagnostics) + expect(error.message).toContain('Checkly configuration is not valid:') + expect(error.message).toContain('[checkly.config.ts] Invalid property value') + expect(error.message).toContain('The value provided for property "bundle" is not valid.') + expect(error.message).not.toContain('Not included in the message.') + }) +}) diff --git a/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js b/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js new file mode 100644 index 00000000..6fe1dee8 --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/embedded-packages-multiple-bad.js @@ -0,0 +1,14 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation of bundle.packages.embed is what rejects it. Contains +// two invalid specs to verify that both are reported in a single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + bundle: { + packages: { + embed: ['Not A Valid Name', 'left-pad@^1.0.0'], + }, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js new file mode 100644 index 00000000..d1ceb874 --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/multiple-errors.js @@ -0,0 +1,13 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// to verify that all of them are reported in a single run. +const config = { + projectName: 'multiple-errors', + // logicalId is missing + bundle: 5, + runner: { + registires: {}, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js b/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js new file mode 100644 index 00000000..5c53019d --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/non-string-fields.js @@ -0,0 +1,9 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. The fields are present but not +// strings, which is a different diagnostic than a missing field. +const config = { + projectName: 42, + logicalId: false, +} + +export default config diff --git a/packages/cli/src/services/checkly-config-loader.ts b/packages/cli/src/services/checkly-config-loader.ts index e189bac7..1d26871a 100644 --- a/packages/cli/src/services/checkly-config-loader.ts +++ b/packages/cli/src/services/checkly-config-loader.ts @@ -5,6 +5,13 @@ import { CheckProps, RuntimeCheckProps } from '../constructs/check.js' import { PlaywrightCheckProps } from '../constructs/playwright-check.js' import { Session } from '../constructs/index.js' import { Construct } from '../constructs/construct.js' +import { Diagnostics } from '../constructs/diagnostics.js' +import { + InvalidPropertyValueDiagnostic, + RequiredPropertyDiagnostic, + UnsupportedPropertyDiagnostic, +} from '../constructs/construct-diagnostics.js' +import { ConfigFileDiagnostics, InvalidConfigError } from './config-diagnostics.js' import type { Region } from '../index.js' import { ReporterType } from '../reporters/reporter.js' import { PlaywrightConfig } from '../constructs/playwright-config.js' @@ -394,10 +401,14 @@ export async function loadChecklyConfig ( filenames = defaultFilenames, writeChecklyConfig: boolean = true, playwrightConfigPath?: string, -): Promise<{ config: ChecklyConfig, constructs: Construct[] }> { +): Promise<{ config: ChecklyConfig, constructs: Construct[], diagnostics: Diagnostics }> { Session.loadingChecklyConfigFile = true try { let config: ChecklyConfig | undefined + // When no config file exists, a default config may be generated in + // memory without ever being written to disk, in which case there is no + // file to attribute diagnostics to. + let configFileName = '' Session.checklyConfigFileConstructs = [] for (const filename of filenames) { const filePath = path.join(dir, filename) @@ -407,15 +418,26 @@ export async function loadChecklyConfig ( continue } config = await Session.loadFile(filePath) + configFileName = path.relative(process.cwd(), filePath) break } if (!config) { config = await handleMissingConfig(dir, filenames, writeChecklyConfig, playwrightConfigPath) + if (writeChecklyConfig) { + // handleMissingConfig() wrote the generated default config to disk. + configFileName = path.relative(process.cwd(), path.join(dir, 'checkly.config.ts')) + } + } + + const diagnostics = new ConfigFileDiagnostics(configFileName) + validateConfigFields(config, ['logicalId', 'projectName'] as const, diagnostics) + validateDependencyCacheVersion(config, diagnostics) + validateBundle(config, diagnostics) + validateRunner(config, diagnostics) + + if (diagnostics.isFatal()) { + throw new InvalidConfigError(diagnostics) } - validateConfigFields(config, ['logicalId', 'projectName'] as const) - validateDependencyCacheVersion(config) - validateBundle(config) - validateRunner(config) const constructs = Session.checklyConfigFileConstructs @@ -423,7 +445,7 @@ export async function loadChecklyConfig ( if (config.cli?.loader) { Session.loader = config.cli.loader } - return { config, constructs } + return { config, constructs, diagnostics } } finally { Session.loadingChecklyConfigFile = false } @@ -447,26 +469,39 @@ async function handleMissingConfig ( throw new ConfigNotFoundError([dir], filenames) } -function validateConfigFields (config: ChecklyConfig, fields: (keyof ChecklyConfig)[]): void { +function validateConfigFields ( + config: ChecklyConfig, + fields: (keyof ChecklyConfig)[], + diagnostics: Diagnostics, +): void { for (const field of fields) { - if (!config?.[field] || !isString(config[field])) { - throw new Error(`Config object missing a ${field} as type string`) + const value = config?.[field] + if (value === undefined || value === null || value === '') { + diagnostics.add(new RequiredPropertyDiagnostic( + field, + new Error(`Value must be a non-empty string.`), + )) + } else if (!isString(value)) { + diagnostics.add(new InvalidPropertyValueDiagnostic( + field, + new Error(`Value must be a non-empty string.`), + )) } } } -function validateDependencyCacheVersion (config: ChecklyConfig): void { +function validateDependencyCacheVersion (config: ChecklyConfig, diagnostics: Diagnostics): void { try { normalizeDependencyCacheVersion(config.caching?.dependencyCache?.version) } catch (cause) { - throw new Error( - `Config field 'caching.dependencyCache.version' must be a string or a safe integer if set`, - { cause }, - ) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'caching.dependencyCache.version', + cause as Error, + )) } } -function validateBundle (config: ChecklyConfig): void { +function validateBundle (config: ChecklyConfig, diagnostics: Diagnostics): void { const { bundle } = config if (bundle === undefined) { return @@ -477,7 +512,11 @@ function validateBundle (config: ChecklyConfig): void { // the runner. Plain-JS configs bypass the TypeScript type, so the shape // must be enforced at runtime. if (bundle === null || typeof bundle !== 'object' || Array.isArray(bundle)) { - throw new Error(`Config field 'bundle' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle', + new Error(`Value must be an object if set.`), + )) + return } const { packages } = bundle @@ -486,13 +525,20 @@ function validateBundle (config: ChecklyConfig): void { } if (packages === null || typeof packages !== 'object' || Array.isArray(packages)) { - throw new Error(`Config field 'bundle.packages' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages', + new Error(`Value must be an object if set.`), + )) + return } try { normalizePackagePrune(packages.prune) } catch (cause) { - throw new Error(`Config field 'bundle.packages.prune' is invalid: ${(cause as Error).message}`, { cause }) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages.prune', + cause as Error, + )) } const embeddedPackages = packages.embed @@ -501,19 +547,26 @@ function validateBundle (config: ChecklyConfig): void { } if (!Array.isArray(embeddedPackages)) { - throw new Error(`Config field 'bundle.packages.embed' must be an array of strings if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages.embed', + new Error(`Value must be an array of strings if set.`), + )) + return } for (const spec of embeddedPackages) { try { parseEmbeddedPackageSpec(spec) } catch (cause) { - throw new Error(`Config field 'bundle.packages.embed' is invalid: ${(cause as Error).message}`, { cause }) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'bundle.packages.embed', + cause as Error, + )) } } } -function validateRunner (config: ChecklyConfig): void { +function validateRunner (config: ChecklyConfig, diagnostics: Diagnostics): void { const { runner } = config if (runner === undefined) { return @@ -524,14 +577,21 @@ function validateRunner (config: ChecklyConfig): void { // `registries: undefined` and silently disable routing, surfacing only as // an install failure on the runner. if (runner === null || typeof runner !== 'object' || Array.isArray(runner)) { - throw new Error(`Config field 'runner' must be an object if set`) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'runner', + new Error(`Value must be an object if set.`), + )) + return } // A misspelled key (`registires`) would silently disable routing the same // way a misshapen block would. for (const key of Object.keys(runner)) { if (key !== 'registries') { - throw new Error(`Config field 'runner' contains unknown field '${key}' (expected only: 'registries')`) + diagnostics.add(new UnsupportedPropertyDiagnostic( + `runner.${key}`, + new Error(`Only 'registries' is supported in the 'runner' block.`), + )) } } @@ -543,6 +603,9 @@ function validateRunner (config: ChecklyConfig): void { try { validateRegistries(registries) } catch (cause) { - throw new Error(`Config field 'runner.registries' is invalid: ${(cause as Error).message}`, { cause }) + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'runner.registries', + cause as Error, + )) } } diff --git a/packages/cli/src/services/config-diagnostics.ts b/packages/cli/src/services/config-diagnostics.ts new file mode 100644 index 00000000..6db7d666 --- /dev/null +++ b/packages/cli/src/services/config-diagnostics.ts @@ -0,0 +1,66 @@ +import { Diagnostic, Diagnostics } from '../constructs/diagnostics.js' + +/** + * Attributes an underlying diagnostic to the Checkly configuration file it + * originates from by prefixing the title with the file name. + */ +export class ConfigFileDiagnostic extends Diagnostic { + underlying: Diagnostic + + constructor (fileName: string, underlying: Diagnostic) { + super({ + title: `[${fileName}] ${underlying.title}`, + message: underlying.message, + }) + + this.underlying = underlying + } + + isFatal (): boolean { + return this.underlying.isFatal() + } + + isBenign (): boolean { + return this.underlying.isBenign() + } +} + +/** + * A Diagnostics collector that attributes every added diagnostic to a + * Checkly configuration file. + */ +export class ConfigFileDiagnostics extends Diagnostics { + fileName: string + + constructor (fileName: string) { + super() + this.fileName = fileName + } + + add (diagnostic: Diagnostic): void { + super.add(new ConfigFileDiagnostic(this.fileName, diagnostic)) + } +} + +/** + * Thrown when the Checkly configuration file contains fatal diagnostics. + * + * The message is composed from the fatal observations so that the error + * remains readable even when rendered by a generic error handler. Callers + * that can render diagnostics should prefer the `diagnostics` field. + */ +export class InvalidConfigError extends Error { + diagnostics: Diagnostics + + constructor (diagnostics: Diagnostics) { + const fatalObservations = diagnostics.observations.filter(diagnostic => diagnostic.isFatal()) + const message = `Checkly configuration is not valid:` + + `\n\n` + + fatalObservations + .map(diagnostic => `${diagnostic.title}\n\n${diagnostic.message}`) + .join('\n\n') + super(message) + this.name = 'InvalidConfigError' + this.diagnostics = diagnostics + } +} From 0207ecdc0c3fcca5df667d9aa6a3f6c6d2473d37 Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 28 Aug 2026 01:51:53 +0900 Subject: [PATCH 2/5] feat(cli): render invalid-config diagnostics from a shared command layer [RED-914] Add CommandStyle.diagnostics() as the single severity-to-renderer dispatch, AuthCommand.validateProject() to absorb the project validation block that was duplicated across five commands, and an InvalidConfigError branch in BaseCommand.catch() that renders the carried config diagnostics and exits 1, giving every command styled output for a fatally invalid config without per-command error handling. Co-Authored-By: Claude Fable 5 --- .../base-command-diagnostics.spec.ts | 82 +++++++++++++++++++ packages/cli/src/commands/authCommand.ts | 33 ++++++++ packages/cli/src/commands/baseCommand.ts | 9 ++ packages/cli/src/helpers/command-style.ts | 13 +++ .../cli/src/services/checkly-config-loader.ts | 10 +++ 5 files changed, 147 insertions(+) create mode 100644 packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts diff --git a/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts b/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts new file mode 100644 index 00000000..b9e3898d --- /dev/null +++ b/packages/cli/src/commands/__tests__/base-command-diagnostics.spec.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi } from 'vitest' + +import { BaseCommand } from '../baseCommand.js' +import { CommandStyle } from '../../helpers/command-style.js' +import { ConfigFileDiagnostics, InvalidConfigError } from '../../services/config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, NoticeDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' + +function createCommandContext () { + let exitCodeValue: number | undefined + return { + style: { + diagnostics: CommandStyle.prototype.diagnostics, + actionFailure: vi.fn(), + longError: vi.fn(), + longWarning: vi.fn(), + longInfo: vi.fn(), + shortError: vi.fn(), + }, + exit: vi.fn((code: number) => { + exitCodeValue = code + throw new Error(`EXIT_${code}`) + }), + catch: (BaseCommand.prototype as any).catch, + get exitCodeValue () { + return exitCodeValue + }, + } +} + +describe('CommandStyle', () => { + describe('diagnostics()', () => { + it('dispatches diagnostics by severity', () => { + const ctx = createCommandContext() + + const diagnostics = new Diagnostics() + diagnostics.add(new ErrorDiagnostic({ + title: 'error title', + message: 'error message', + error: new Error('error message'), + })) + diagnostics.add(new WarningDiagnostic({ + title: 'warning title', + message: 'warning message', + })) + diagnostics.add(new NoticeDiagnostic({ + title: 'notice title', + message: 'notice message', + })) + + ctx.style.diagnostics(diagnostics) + + expect(ctx.style.longError).toHaveBeenCalledWith('error title', 'error message') + expect(ctx.style.longWarning).toHaveBeenCalledWith('warning title', 'warning message') + expect(ctx.style.longInfo).toHaveBeenCalledWith('notice title', 'notice message') + }) + }) +}) + +describe('BaseCommand', () => { + describe('catch()', () => { + it('renders config diagnostics and exits 1 on InvalidConfigError', () => { + const ctx = createCommandContext() + + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Invalid property value', + message: 'The value is not valid.', + error: new Error('The value is not valid.'), + })) + + expect(() => ctx.catch.call(ctx, new InvalidConfigError(diagnostics))) + .toThrow('EXIT_1') + + expect(ctx.style.longError).toHaveBeenCalledWith( + '[checkly.config.ts] Invalid property value', + 'The value is not valid.', + ) + expect(ctx.style.shortError).toHaveBeenCalledWith('Your Checkly configuration file is not valid.') + expect(ctx.exitCodeValue).toBe(1) + }) + }) +}) diff --git a/packages/cli/src/commands/authCommand.ts b/packages/cli/src/commands/authCommand.ts index d93a0aca..d7b276b3 100644 --- a/packages/cli/src/commands/authCommand.ts +++ b/packages/cli/src/commands/authCommand.ts @@ -3,7 +3,9 @@ import { BaseCommand } from './baseCommand.js' import * as api from '../rest/api.js' import { Account } from '../rest/accounts.js' import { Session } from '../constructs/session.js' +import { Diagnostics } from '../constructs/diagnostics.js' import { detectCliMode } from '../helpers/cli-mode.js' +import type { Project } from '../constructs/project.js' import type { CommandPreview } from '../helpers/command-preview.js' import { formatPreviewForAgent, formatPreviewForTerminal } from '../helpers/command-preview.js' @@ -28,6 +30,37 @@ export abstract class AuthCommand extends BaseCommand { Session.accountFeatures = this.#account?.features ?? [] } + protected async validateProject ( + project: Project, + options: { + configDiagnostics: Diagnostics + failureMessage?: string + }, + ): Promise { + const { + configDiagnostics, + failureMessage = `Unable to continue due to unresolved validation errors.`, + } = options + + this.style.actionStart('Validating project resources') + + const diagnostics = new Diagnostics() + // Config diagnostics come first so that they render before any + // project-level diagnostics. + diagnostics.extend(configDiagnostics) + await project.validate(diagnostics) + + this.style.diagnostics(diagnostics) + + if (diagnostics.isFatal()) { + this.style.actionFailure() + this.style.shortError(failureMessage) + this.exit(1) + } + + this.style.actionSuccess() + } + protected async confirmOrAbort ( preview: CommandPreview, options: { force: boolean, dryRun?: boolean, interactiveConfirm?: () => Promise }, diff --git a/packages/cli/src/commands/baseCommand.ts b/packages/cli/src/commands/baseCommand.ts index 6919ce6e..6c3ba716 100644 --- a/packages/cli/src/commands/baseCommand.ts +++ b/packages/cli/src/commands/baseCommand.ts @@ -6,6 +6,7 @@ import { dirname, relative } from 'node:path' import { api } from '../rest/api.js' import { assignProxy } from '../services/proxy.js' import { CommandStyle } from '../helpers/command-style.js' +import { InvalidConfigError } from '../services/config-diagnostics.js' import { findStaleSkills } from '../services/skills.js' import { PackageJsonFile } from '../services/check-parser/package-files/package-json-file.js' import { detectNearestPackageJson } from '../services/check-parser/package-files/package-manager.js' @@ -152,6 +153,14 @@ export abstract class BaseCommand extends Command { } protected catch (err: Error & { exitCode?: number }): Promise { + if (err instanceof InvalidConfigError) { + // Stops the spinner if one is running (e.g. `validate` starts one + // before loading the config); also emits a blank separator line. + this.style.actionFailure() + this.style.diagnostics(err.diagnostics) + this.style.shortError(`Your Checkly configuration file is not valid.`) + return this.exit(1) + } // TODO: we can add Sentry here and log critical errors. return super.catch(err) } diff --git a/packages/cli/src/helpers/command-style.ts b/packages/cli/src/helpers/command-style.ts index 3e25c6b1..d1462aaa 100644 --- a/packages/cli/src/helpers/command-style.ts +++ b/packages/cli/src/helpers/command-style.ts @@ -2,6 +2,7 @@ import chalk from 'chalk' import { ux } from '@oclif/core' import { BaseCommand } from '../commands/baseCommand.js' +import { Diagnostics } from '../constructs/diagnostics.js' import { wrap } from './wrap.js' import logSymbols from 'log-symbols' @@ -110,6 +111,18 @@ export class CommandStyle { this.c.log() } + diagnostics (diagnostics: Diagnostics) { + for (const diag of diagnostics.observations) { + if (diag.isFatal()) { + this.longError(diag.title, diag.message) + } else if (!diag.isBenign()) { + this.longWarning(diag.title, diag.message) + } else { + this.longInfo(diag.title, diag.message) + } + } + } + fatal (message: string) { this.c.log(chalk.red(message)) this.c.log() diff --git a/packages/cli/src/services/checkly-config-loader.ts b/packages/cli/src/services/checkly-config-loader.ts index 1d26871a..da532b7b 100644 --- a/packages/cli/src/services/checkly-config-loader.ts +++ b/packages/cli/src/services/checkly-config-loader.ts @@ -396,6 +396,16 @@ export const defaultFilenames = [ 'checkly.config.cjs', ] +/** + * Loads and validates the Checkly config file. + * + * All validation issues are collected as diagnostics attributed to the + * config file. When any of them is fatal, loading fails with an + * {@link InvalidConfigError} carrying the diagnostics. The returned + * diagnostics therefore only ever contain non-fatal observations; callers + * that render diagnostics should seed their collector with them ahead of + * project-level diagnostics so that config issues render first. + */ export async function loadChecklyConfig ( dir: string, filenames = defaultFilenames, From 5b0d3f6ba4af685967ede871614a6e4bdbf490eb Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 28 Aug 2026 01:52:06 +0900 Subject: [PATCH 3/5] feat(cli): fail commands on invalid config and render config diagnostics first [RED-914] Wire the diagnostics returned by loadChecklyConfig into every command that loads a config: deploy, test, pw-test, validate and import plan seed them into project validation so config diagnostics render ahead of project diagnostics, while destroy, trigger and the other import commands render them right after loading. Fatal config diagnostics abort every command via the shared InvalidConfigError handling. trigger no longer silently ignores an invalid config (other load failures, including a missing config, are still tolerated). debug parse-project keeps its machine-readable JSON contract for an invalid config, emitting the diagnostics with a null payload, and preserves its non-zero exit code for any other load failure. Co-Authored-By: Claude Fable 5 --- .../__tests__/confirm-flow-deploy.spec.ts | 26 ++-- .../__tests__/confirm-flow-destroy.spec.ts | 40 +++++- .../deploy-config-diagnostics.spec.ts | 126 ++++++++++++++++++ .../__tests__/trigger-config-errors.spec.ts | 86 ++++++++++++ .../__tests__/parse-project-config.spec.ts | 69 ++++++++++ .../invalid-config/checkly.config.js | 9 ++ .../invalid-config/package.json | 6 + .../missing-config/package.json | 6 + .../cli/src/commands/debug/parse-project.ts | 56 ++++++-- packages/cli/src/commands/deploy.ts | 26 +--- packages/cli/src/commands/destroy.ts | 6 +- .../commands/import/__tests__/apply.spec.ts | 3 + .../commands/import/__tests__/cancel.spec.ts | 6 + .../commands/import/__tests__/commit.spec.ts | 4 + packages/cli/src/commands/import/apply.ts | 2 + packages/cli/src/commands/import/cancel.ts | 2 + packages/cli/src/commands/import/commit.ts | 2 + packages/cli/src/commands/import/plan.ts | 39 ++---- packages/cli/src/commands/pw-test.ts | 26 +--- packages/cli/src/commands/test.ts | 26 +--- packages/cli/src/commands/trigger.ts | 13 +- packages/cli/src/commands/validate.ts | 28 +--- 22 files changed, 456 insertions(+), 151 deletions(-) create mode 100644 packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts create mode 100644 packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts create mode 100644 packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts create mode 100644 packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js create mode 100644 packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json create mode 100644 packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json diff --git a/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts b/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts index ce37f1c1..9bc4179c 100644 --- a/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts +++ b/packages/cli/src/commands/__tests__/confirm-flow-deploy.spec.ts @@ -11,17 +11,21 @@ vi.mock('../../rest/api', () => ({ validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), })) -vi.mock('../../services/checkly-config-loader', () => ({ - loadChecklyConfig: vi.fn().mockResolvedValue({ - config: { - logicalId: 'my-project', - projectName: 'My Project', - repoUrl: 'https://github.com/checkly/checkly-cli', - checks: {}, - }, - constructs: [], - }), -})) +vi.mock('../../services/checkly-config-loader', async () => { + const { Diagnostics } = await import('../../constructs/diagnostics.js') + return { + loadChecklyConfig: vi.fn().mockResolvedValue({ + config: { + logicalId: 'my-project', + projectName: 'My Project', + repoUrl: 'https://github.com/checkly/checkly-cli', + checks: {}, + }, + constructs: [], + diagnostics: new Diagnostics(), + }), + } +}) vi.mock('../../services/project-parser', () => ({ parseProject: vi.fn(), diff --git a/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts b/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts index bb41fce5..cca2eee1 100644 --- a/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts +++ b/packages/cli/src/commands/__tests__/confirm-flow-destroy.spec.ts @@ -10,11 +10,15 @@ vi.mock('../../rest/api', () => ({ validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), })) -vi.mock('../../services/checkly-config-loader', () => ({ - loadChecklyConfig: vi.fn().mockResolvedValue({ - config: { logicalId: 'my-project', projectName: 'My Project' }, - }), -})) +vi.mock('../../services/checkly-config-loader', async () => { + const { Diagnostics } = await import('../../constructs/diagnostics.js') + return { + loadChecklyConfig: vi.fn().mockResolvedValue({ + config: { logicalId: 'my-project', projectName: 'My Project' }, + diagnostics: new Diagnostics(), + }), + } +}) vi.mock('../../services/util', () => ({ splitConfigFilePath: vi.fn().mockReturnValue({ @@ -31,6 +35,8 @@ import { detectCliMode } from '../../helpers/cli-mode.js' import prompts from 'prompts' import * as api from '../../rest/api.js' import { buildConfirmCommand } from '../../helpers/command-preview.js' +import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { Diagnostics, WarningDiagnostic } from '../../constructs/diagnostics.js' import { AuthCommand } from '../authCommand.js' import Destroy from '../destroy.js' @@ -60,6 +66,7 @@ function createCommandContext (parsed: { flags: Record, metadat confirmOrAbort: AuthCommand.prototype.confirmOrAbort, style: { outputFormat: undefined, + diagnostics: vi.fn(), longError: vi.fn(), actionStart: vi.fn(), actionStatus: vi.fn(), @@ -98,6 +105,29 @@ describe('destroy confirmation flow', () => { expect(api.projects.deleteProject).not.toHaveBeenCalled() }) + it('renders non-fatal config diagnostics right after loading the config', async () => { + vi.mocked(detectCliMode).mockReturnValue('agent') + const configDiagnostics = new Diagnostics() + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + vi.mocked(loadChecklyConfig).mockResolvedValueOnce({ + config: { logicalId: 'my-project', projectName: 'My Project' }, + diagnostics: configDiagnostics, + } as any) + const ctx = createCommandContext({ + flags: { force: false }, + }) + + // The confirmation gate exits 2, but the diagnostics render before it. + await expect( + Destroy.prototype.run.call(ctx as any), + ).rejects.toThrow('EXIT_2') + + expect(ctx.style.diagnostics).toHaveBeenCalledWith(configDiagnostics) + }) + it('executes with --force in agent mode', async () => { vi.mocked(detectCliMode).mockReturnValue('agent') const ctx = createCommandContext({ diff --git a/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts b/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts new file mode 100644 index 00000000..cc306306 --- /dev/null +++ b/packages/cli/src/commands/__tests__/deploy-config-diagnostics.spec.ts @@ -0,0 +1,126 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('../../helpers/cli-mode', () => ({ + detectCliMode: vi.fn(() => 'agent'), +})) + +vi.mock('../../rest/api', () => ({ + runtimes: { getAll: vi.fn().mockResolvedValue([]) }, + validateAuthentication: vi.fn().mockResolvedValue({ name: 'Test Account' }), +})) + +vi.mock('../../services/checkly-config-loader', () => ({ + loadChecklyConfig: vi.fn(), +})) + +vi.mock('../../services/project-parser', () => ({ + parseProject: vi.fn(), +})) + +vi.mock('../../services/util', () => ({ + splitConfigFilePath: vi.fn().mockReturnValue({ + configDirectory: '.', + configFilenames: ['checkly.config.ts'], + }), + getGitInformation: vi.fn(), +})) + +import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { parseProject } from '../../services/project-parser.js' +import { ConfigFileDiagnostics } from '../../services/config-diagnostics.js' +import { Diagnostics, ErrorDiagnostic, WarningDiagnostic } from '../../constructs/diagnostics.js' +import { CommandStyle } from '../../helpers/command-style.js' +import { AuthCommand } from '../authCommand.js' +import Deploy from '../deploy.js' + +function createCommandContext (parsed: unknown) { + let exitCodeValue: number | undefined + return { + parse: vi.fn().mockResolvedValue(parsed), + exit: vi.fn((code: number) => { + exitCodeValue = code + throw new Error(`EXIT_${code}`) + }), + style: { + outputFormat: undefined, + diagnostics: CommandStyle.prototype.diagnostics, + actionStart: vi.fn(), + actionSuccess: vi.fn(), + actionFailure: vi.fn(), + longError: vi.fn(), + longWarning: vi.fn(), + longInfo: vi.fn(), + shortError: vi.fn(), + }, + validateProject: (AuthCommand.prototype as any).validateProject, + constructor: Deploy, + account: { name: 'Test Account', runtimeId: 'runtime-default' }, + get exitCodeValue () { + return exitCodeValue + }, + } +} + +const deployFlags = { + flags: { + 'force': true, + 'preview': true, + 'output': false, + 'verbose': false, + 'config': undefined, + 'schedule-on-deploy': true, + 'verify-runtime-dependencies': true, + 'debug-bundle': false, + 'debug-bundle-output-file': './debug-bundle.json', + }, + metadata: { flags: {} }, +} + +describe('deploy config diagnostics', () => { + it('renders config diagnostics ahead of project diagnostics', async () => { + const configDiagnostics = new ConfigFileDiagnostics('checkly.config.ts') + configDiagnostics.add(new WarningDiagnostic({ + title: 'Config warning', + message: 'A config-level warning.', + })) + vi.mocked(loadChecklyConfig).mockResolvedValue({ + config: { + logicalId: 'my-project', + projectName: 'My Project', + checks: {}, + }, + constructs: [], + diagnostics: configDiagnostics, + } as any) + + const project = { + repoUrl: undefined, + validate: vi.fn((diagnostics: Diagnostics) => { + diagnostics.add(new ErrorDiagnostic({ + title: 'Project error', + message: 'A project-level error.', + error: new Error('A project-level error.'), + })) + return Promise.resolve() + }), + } + vi.mocked(parseProject).mockResolvedValue(project as any) + + const ctx = createCommandContext(deployFlags) + + await expect(Deploy.prototype.run.call(ctx as any)).rejects.toThrow('EXIT_1') + + expect(ctx.style.longWarning).toHaveBeenCalledWith( + '[checkly.config.ts] Config warning', + 'A config-level warning.', + ) + expect(ctx.style.longError).toHaveBeenCalledWith('Project error', 'A project-level error.') + + // Config diagnostics must render before project diagnostics. + const warningOrder = ctx.style.longWarning.mock.invocationCallOrder[0] + const errorOrder = ctx.style.longError.mock.invocationCallOrder[0] + expect(warningOrder).toBeLessThan(errorOrder) + + expect(ctx.exitCodeValue).toBe(1) + }) +}) diff --git a/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts b/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts new file mode 100644 index 00000000..e2ece388 --- /dev/null +++ b/packages/cli/src/commands/__tests__/trigger-config-errors.spec.ts @@ -0,0 +1,86 @@ +import { describe, expect, it, vi } from 'vitest' + +vi.mock('../../services/checkly-config-loader', async importOriginal => { + // Keep the real ConfigNotFoundError so instanceof checks work. + const actual = await importOriginal() + return { + ...actual, + loadChecklyConfig: vi.fn(), + } +}) + +vi.mock('../../services/util', () => ({ + splitConfigFilePath: vi.fn().mockReturnValue({ + configDirectory: '.', + configFilenames: ['checkly.config.ts'], + }), + getEnvs: vi.fn().mockResolvedValue({}), + getGitInformation: vi.fn(), + getCiInformation: vi.fn(), +})) + +import { loadChecklyConfig, ConfigNotFoundError } from '../../services/checkly-config-loader.js' +import { ConfigFileDiagnostics, InvalidConfigError } from '../../services/config-diagnostics.js' +import { ErrorDiagnostic } from '../../constructs/diagnostics.js' +import Trigger from '../trigger.js' + +// The harness stops the run right after the config-load step, which is the +// behavior under test: which load failures are tolerated and which abort. +class StopError extends Error {} + +function createCommandContext () { + return { + parse: vi.fn().mockResolvedValue({ + flags: { + 'config': undefined, + 'env': [], + 'env-file': undefined, + 'reporter': undefined, + 'retries': undefined, + 'detach': false, + }, + }), + style: { + diagnostics: vi.fn(), + }, + prepareRunLocation: vi.fn(() => { + throw new StopError('reached run preparation') + }), + constructor: Trigger, + } +} + +describe('trigger config-load failures', () => { + it('tolerates a missing config file', async () => { + vi.mocked(loadChecklyConfig).mockRejectedValue( + new ConfigNotFoundError(['.'], ['checkly.config.ts']), + ) + const ctx = createCommandContext() + + // Reaching run preparation means the missing config was swallowed. + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toThrow(StopError) + }) + + it('tolerates a config that fails to load', async () => { + vi.mocked(loadChecklyConfig).mockRejectedValue( + new Error(`Error loading file 'checkly.config.ts'`), + ) + const ctx = createCommandContext() + + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toThrow(StopError) + }) + + it('fails on an invalid config', async () => { + const diagnostics = new ConfigFileDiagnostics('checkly.config.ts') + diagnostics.add(new ErrorDiagnostic({ + title: 'Missing required property', + message: 'Property "logicalId" is required and must be set.', + error: new Error('Property "logicalId" is required and must be set.'), + })) + vi.mocked(loadChecklyConfig).mockRejectedValue(new InvalidConfigError(diagnostics)) + const ctx = createCommandContext() + + await expect(Trigger.prototype.run.call(ctx as any)).rejects.toBeInstanceOf(InvalidConfigError) + expect(ctx.prepareRunLocation).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts b/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts new file mode 100644 index 00000000..8ab14e36 --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-config.spec.ts @@ -0,0 +1,69 @@ +import path from 'node:path' + +import { describe, it, expect, beforeAll, afterAll } from 'vitest' + +import { FixtureSandbox } from '../../../testing/fixture-sandbox.js' +import { ParseProjectOutput } from '../parse-project.js' + +const DEFAULT_FIXT_TIMEOUT = 180_000 + +describe('debug parse-project config handling', () => { + describe('invalid config', () => { + let fixt: FixtureSandbox + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + source: path.join(__dirname, 'parse-project-fixtures', 'invalid-config'), + }) + }, DEFAULT_FIXT_TIMEOUT) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('emits the machine-readable diagnostics shape with a null payload', async () => { + const result = await fixt.run('pnpm', ['checkly', 'debug', 'parse-project']) + + expect(result.exitCode).toBe(0) + + const output: ParseProjectOutput = JSON.parse(result.stdout) + expect(output.payload).toBeNull() + expect(output.diagnostics.fatal).toBe(true) + expect(output.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('Missing required property'), + fatal: true, + }), + expect.objectContaining({ + title: expect.stringContaining('Invalid property value'), + fatal: true, + }), + ])) + }) + }) + + describe('missing config', () => { + let fixt: FixtureSandbox + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + source: path.join(__dirname, 'parse-project-fixtures', 'missing-config'), + }) + }, DEFAULT_FIXT_TIMEOUT) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('fails with a non-zero exit code', async () => { + // fixt.run() rejects on a non-zero exit; the execa error carries the + // exit code and output. An unexpected success resolves with exit code + // 0 and fails the assertion below. + const result = await fixt.run('pnpm', ['checkly', 'debug', 'parse-project']) + .catch((err: any) => err) + + expect(result.exitCode).not.toBe(0) + expect(result.stderr).toContain('Unable to detect a Checkly configuration file') + }) + }) +}) diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js new file mode 100644 index 00000000..f4102893 --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/checkly.config.js @@ -0,0 +1,9 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. +const config = { + projectName: 'invalid-config', + // logicalId is missing + bundle: 5, +} + +export default config diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json new file mode 100644 index 00000000..5b7f4c96 --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/invalid-config/package.json @@ -0,0 +1,6 @@ +{ + "name": "invalid-config", + "type": "module", + "version": "1.0.0", + "private": true +} diff --git a/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json new file mode 100644 index 00000000..30dbef4d --- /dev/null +++ b/packages/cli/src/commands/debug/__tests__/parse-project-fixtures/missing-config/package.json @@ -0,0 +1,6 @@ +{ + "name": "missing-config", + "type": "module", + "version": "1.0.0", + "private": true +} diff --git a/packages/cli/src/commands/debug/parse-project.ts b/packages/cli/src/commands/debug/parse-project.ts index 0d17446c..94e6ad18 100644 --- a/packages/cli/src/commands/debug/parse-project.ts +++ b/packages/cli/src/commands/debug/parse-project.ts @@ -2,6 +2,7 @@ import { Command, Flags } from '@oclif/core' import { parseProject } from '../../services/project-parser.js' import { loadChecklyConfig } from '../../services/checkly-config-loader.js' +import { InvalidConfigError } from '../../services/config-diagnostics.js' import { Diagnostics, Session, @@ -37,6 +38,21 @@ export type ParseProjectOutput = { member: boolean payload: unknown }[] + } | null +} + +function serializeDiagnostics (diagnostics: Diagnostics): ParseProjectOutput['diagnostics'] { + return { + fatal: diagnostics.isFatal(), + benign: diagnostics.isBenign(), + observations: diagnostics.observations.map(diag => { + return { + title: diag.title, + message: diag.message, + fatal: diag.isFatal(), + benign: diag.isBenign(), + } + }), } } @@ -111,10 +127,33 @@ export default class ParseProjectCommand extends Command { : undefined sampler?.unref() const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) + + let loaded + try { + loaded = await loadChecklyConfig(configDirectory, configFilenames) + } catch (err) { + if (err instanceof InvalidConfigError) { + // Keep the machine-readable diagnostics contract for a fatally + // invalid config instead of degrading to a generic error. + const output: ParseProjectOutput = { + diagnostics: serializeDiagnostics(err.diagnostics), + payload: null, + } + + // eslint-disable-next-line no-console + console.log(JSON.stringify(output, null, 2)) + return + } + + // Any other load failure (e.g. a missing config file) escapes to + // oclif's default handler, preserving its non-zero exit code. + throw err + } const { config: checklyConfig, constructs: checklyConfigConstructs, - } = await loadChecklyConfig(configDirectory, configFilenames) + diagnostics: configDiagnostics, + } = loaded const availableRuntimes = await loadSnapshot() try { @@ -157,6 +196,8 @@ export default class ParseProjectCommand extends Command { const parseMs = performance.now() - parseStartedAt const diagnostics = new Diagnostics() + // Config diagnostics come first so that they lead the output. + diagnostics.extend(configDiagnostics) await project.validate(diagnostics) let bundleMs = 0 @@ -188,18 +229,7 @@ export default class ParseProjectCommand extends Command { })() const output = { - diagnostics: { - fatal: diagnostics.isFatal(), - benign: diagnostics.isBenign(), - observations: diagnostics.observations.map(diag => { - return { - title: diag.title, - message: diag.message, - fatal: diag.isFatal(), - benign: diag.isBenign(), - } - }), - }, + diagnostics: serializeDiagnostics(diagnostics), payload, } diff --git a/packages/cli/src/commands/deploy.ts b/packages/cli/src/commands/deploy.ts index f1aa16ff..20a29897 100644 --- a/packages/cli/src/commands/deploy.ts +++ b/packages/cli/src/commands/deploy.ts @@ -8,7 +8,7 @@ import { loadChecklyConfig } from '../services/checkly-config-loader.js' import { Check, AlertChannelSubscription, AlertChannel, CheckGroup, Dashboard, MaintenanceWindow, PrivateLocation, PrivateLocationCheckAssignment, PrivateLocationGroupAssignment, - Project, ProjectData, Diagnostics, + Project, ProjectData, Session, StatusPage, StatusPageService, } from '../constructs/index.js' import chalk from 'chalk' @@ -129,6 +129,7 @@ export default class Deploy extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const account = this.account @@ -185,28 +186,7 @@ export default class Deploy extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/destroy.ts b/packages/cli/src/commands/destroy.ts index 4ec85eaa..48e36b75 100644 --- a/packages/cli/src/commands/destroy.ts +++ b/packages/cli/src/commands/destroy.ts @@ -38,7 +38,11 @@ export default class Destroy extends AuthCommand { 'cancel-in-progress-deployment': cancelInProgress, } = flags const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) - const { config: checklyConfig } = await loadChecklyConfig(configDirectory, configFilenames) + const { + config: checklyConfig, + diagnostics: configDiagnostics, + } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const account = this.account await this.confirmOrAbort({ diff --git a/packages/cli/src/commands/import/__tests__/apply.spec.ts b/packages/cli/src/commands/import/__tests__/apply.spec.ts index fa34dcd2..8310186c 100644 --- a/packages/cli/src/commands/import/__tests__/apply.spec.ts +++ b/packages/cli/src/commands/import/__tests__/apply.spec.ts @@ -19,6 +19,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportApplyCommand from '../apply.js' @@ -39,6 +40,7 @@ function createCommandContext (parsed: unknown) { throw new Error(`EXIT_${code}`) }), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -57,6 +59,7 @@ describe('import apply command (non-interactive)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.applyImportPlan).mockResolvedValue({} as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) diff --git a/packages/cli/src/commands/import/__tests__/cancel.spec.ts b/packages/cli/src/commands/import/__tests__/cancel.spec.ts index f5db9479..2b0846c0 100644 --- a/packages/cli/src/commands/import/__tests__/cancel.spec.ts +++ b/packages/cli/src/commands/import/__tests__/cancel.spec.ts @@ -18,6 +18,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportCancelCommand from '../cancel.js' @@ -54,6 +55,7 @@ function createCommandContext (parsed: unknown) { }), confirmOrAbort: vi.fn(), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -73,6 +75,7 @@ describe('import cancel command (non-interactive)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) @@ -147,6 +150,7 @@ describe('import cancel command (flag validation)', () => { vi.clearAllMocks() vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) @@ -180,6 +184,7 @@ describe('import cancel command (empty candidate list)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.findImportPlans).mockResolvedValue({ data: [] } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) @@ -221,6 +226,7 @@ describe('import cancel command (confirmation gate)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.cancelImportPlan).mockResolvedValue({} as any) }) diff --git a/packages/cli/src/commands/import/__tests__/commit.spec.ts b/packages/cli/src/commands/import/__tests__/commit.spec.ts index 18d27a79..fe13aebf 100644 --- a/packages/cli/src/commands/import/__tests__/commit.spec.ts +++ b/packages/cli/src/commands/import/__tests__/commit.spec.ts @@ -18,6 +18,7 @@ vi.mock('../../../services/checkly-config-loader', () => ({ import { detectCliMode } from '../../../helpers/cli-mode.js' import * as api from '../../../rest/api.js' import { loadChecklyConfig } from '../../../services/checkly-config-loader.js' +import { Diagnostics } from '../../../constructs/diagnostics.js' import { ImportPlan } from '../../../rest/projects.js' import ImportCommitCommand from '../commit.js' @@ -52,6 +53,7 @@ function createCommandContext (parsed: unknown) { }), confirmOrAbort: vi.fn(), style: { + diagnostics: vi.fn(), actionStart: vi.fn(), actionSuccess: vi.fn(), actionFailure: vi.fn(), @@ -68,6 +70,7 @@ describe('import commit command (non-interactive)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) }) @@ -132,6 +135,7 @@ describe('import commit command (confirmation gate)', () => { vi.mocked(detectCliMode).mockReturnValue('agent') vi.mocked(loadChecklyConfig).mockResolvedValue({ config: { logicalId: 'my-project' }, + diagnostics: new Diagnostics(), } as any) vi.mocked(api.projects.commitImportPlan).mockResolvedValue({} as any) }) diff --git a/packages/cli/src/commands/import/apply.ts b/packages/cli/src/commands/import/apply.ts index 9919633e..586349b0 100644 --- a/packages/cli/src/commands/import/apply.ts +++ b/packages/cli/src/commands/import/apply.ts @@ -42,7 +42,9 @@ export default class ImportApplyCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/cancel.ts b/packages/cli/src/commands/import/cancel.ts index 0c3a206b..8832c0aa 100644 --- a/packages/cli/src/commands/import/cancel.ts +++ b/packages/cli/src/commands/import/cancel.ts @@ -49,7 +49,9 @@ export default class ImportCancelCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/commit.ts b/packages/cli/src/commands/import/commit.ts index 1e2aa496..7c59c1bc 100644 --- a/packages/cli/src/commands/import/commit.ts +++ b/packages/cli/src/commands/import/commit.ts @@ -41,7 +41,9 @@ export default class ImportCommitCommand extends AuthCommand { const { configDirectory, configFilenames } = splitConfigFilePath(configFilename) const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.style.diagnostics(configDiagnostics) const { logicalId, diff --git a/packages/cli/src/commands/import/plan.ts b/packages/cli/src/commands/import/plan.ts index 0a338567..d9330ab8 100644 --- a/packages/cli/src/commands/import/plan.ts +++ b/packages/cli/src/commands/import/plan.ts @@ -141,6 +141,13 @@ future deployments include the imported resources.` 'import', ] + /** + * Non-fatal diagnostics from loading the Checkly config file, rendered + * ahead of project-level diagnostics. Empty when no config file exists + * (e.g. the config was created interactively). + */ + readonly #configDiagnostics = new Diagnostics() + async run (): Promise { const { flags, argv } = await this.parse(ImportPlanCommand) const { @@ -897,8 +904,11 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co try { const { config: checklyConfig, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) + this.#configDiagnostics.extend(configDiagnostics) + return checklyConfig } catch (err) { if (err instanceof ConfigNotFoundError) { @@ -909,31 +919,6 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co } } - async #validateProject (project: Project): Promise { - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() - } - async #findExportedResources ( configDirectory: string, checklyConfig: ChecklyConfig, @@ -972,7 +957,9 @@ ${chalk.cyan('For safety, resources are not deletable until the plan has been co throw err } - await this.#validateProject(project) + await this.validateProject(project, { + configDiagnostics: this.#configDiagnostics, + }) this.style.actionStart('Searching for exported resources') diff --git a/packages/cli/src/commands/pw-test.ts b/packages/cli/src/commands/pw-test.ts index 897064da..3fa559e6 100644 --- a/packages/cli/src/commands/pw-test.ts +++ b/packages/cli/src/commands/pw-test.ts @@ -12,7 +12,7 @@ import { prepareReportersTypes, prepareRunLocation, splitChecklyAndPlaywrightFla import * as api from '../rest/api.js' import config from '../services/config.js' import { parseProject } from '../services/project-parser.js' -import { Diagnostics, PlaywrightCheck, RuntimeCheck, Session } from '../constructs/index.js' +import { PlaywrightCheck, RuntimeCheck, Session } from '../constructs/index.js' import { Flags } from '@oclif/core' import { createReporters, ReporterType } from '../reporters/reporter.js' import TestRunner from '../services/test-runner.js' @@ -156,6 +156,7 @@ export default class PwTestCommand extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames, false, pwPathFlag) let playwrightConfigPath = pwPathFlag ?? checklyConfig.checks?.playwrightConfigPath @@ -242,28 +243,7 @@ export default class PwTestCommand extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/test.ts b/packages/cli/src/commands/test.ts index c1efdec9..52f01078 100644 --- a/packages/cli/src/commands/test.ts +++ b/packages/cli/src/commands/test.ts @@ -12,7 +12,7 @@ import TestRunner from '../services/test-runner.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' import { filterByFileNamePattern, filterByCheckNamePattern, filterByTags } from '../services/test-filters.js' import { AuthCommand } from './authCommand.js' -import { BrowserCheck, Check, Diagnostics, HeartbeatMonitor, MultiStepCheck, Project, RetryStrategyBuilder, RuntimeCheck, Session } from '../constructs/index.js' +import { BrowserCheck, Check, HeartbeatMonitor, MultiStepCheck, Project, RetryStrategyBuilder, RuntimeCheck, Session } from '../constructs/index.js' import type { Region } from '../index.js' import { splitConfigFilePath, getGitInformation, getCiInformation, getEnvs } from '../services/util.js' import { createReporters, ReporterType } from '../reporters/reporter.js' @@ -169,6 +169,7 @@ export default class Test extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const location = await prepareRunLocation(checklyConfig.cli, { @@ -267,28 +268,7 @@ export default class Test extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Unable to continue due to unresolved validation errors.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { configDiagnostics }) const bundler = await Bundler.createForWorkspace(Session.workspace.unwrap(), { dependencyCacheVersion: checklyConfig.caching?.dependencyCache?.version, diff --git a/packages/cli/src/commands/trigger.ts b/packages/cli/src/commands/trigger.ts index 9632107d..c3c00dac 100644 --- a/packages/cli/src/commands/trigger.ts +++ b/packages/cli/src/commands/trigger.ts @@ -4,6 +4,7 @@ import { isCI } from 'ci-info' import * as api from '../rest/api.js' import { AuthCommand } from './authCommand.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' +import { InvalidConfigError } from '../services/config-diagnostics.js' import { splitConfigFilePath, getEnvs, getGitInformation, getCiInformation } from '../services/util.js' import type { Region } from '../index.js' import TriggerRunner from '../services/trigger-runner.js' @@ -140,10 +141,16 @@ export default class Trigger extends AuthCommand { let checklyConfig try { - const { config } = await loadChecklyConfig(configDirectory, configFilenames) + const { config, diagnostics: configDiagnostics } = await loadChecklyConfig(configDirectory, configFilenames) checklyConfig = config - } catch { - // Don't throw an error if the config file is missing + this.style.diagnostics(configDiagnostics) + } catch (err) { + // Trigger works without a project, so config-load failures are + // tolerated - except an invalid config, which should be fixed rather + // than silently ignored. + if (err instanceof InvalidConfigError) { + throw err + } } const location = await this.prepareRunLocation(checklyConfig?.cli, { runLocation: runLocation as keyof Region, diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index b06c1ef4..1a561fcb 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -3,7 +3,6 @@ import { Flags } from '@oclif/core' import { AuthCommand } from './authCommand.js' import { parseProject } from '../services/project-parser.js' import { loadChecklyConfig } from '../services/checkly-config-loader.js' -import { Diagnostics } from '../constructs/index.js' import { splitConfigFilePath } from '../services/util.js' import commonMessages from '../messages/common-messages.js' import { Runtime } from '../runtimes/index.js' @@ -39,6 +38,7 @@ export default class Validate extends AuthCommand { const { config: checklyConfig, constructs: checklyConfigConstructs, + diagnostics: configDiagnostics, } = await loadChecklyConfig(configDirectory, configFilenames) const account = this.account const availableRuntimes = await api.runtimes.getAll() @@ -68,28 +68,10 @@ export default class Validate extends AuthCommand { this.style.actionSuccess() - this.style.actionStart('Validating project resources') - - const diagnostics = new Diagnostics() - await project.validate(diagnostics) - - for (const diag of diagnostics.observations) { - if (diag.isFatal()) { - this.style.longError(diag.title, diag.message) - } else if (!diag.isBenign()) { - this.style.longWarning(diag.title, diag.message) - } else { - this.style.longInfo(diag.title, diag.message) - } - } - - if (diagnostics.isFatal()) { - this.style.actionFailure() - this.style.shortError(`Your project is not valid.`) - this.exit(1) - } - - this.style.actionSuccess() + await this.validateProject(project, { + configDiagnostics, + failureMessage: `Your project is not valid.`, + }) this.style.shortSuccess(`Your project is valid.`) } From bd0ec040156ed5cf4b400c72f4d92344bb62a32a Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 28 Aug 2026 02:33:40 +0900 Subject: [PATCH 4/5] test(cli): pass EXECUTION_ID to trigger e2e invocations [RED-914] The trigger fixture config derives its logicalId from EXECUTION_ID, and runCheckly runs with extendEnv: false, so invocations that omit the variable load a config with no logicalId. Trigger aborts on an invalid config, so every invocation must pass the variable, not just the deploy/destroy setup steps. Co-Authored-By: Claude Fable 5 --- packages/cli/e2e/__tests__/trigger.spec.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/cli/e2e/__tests__/trigger.spec.ts b/packages/cli/e2e/__tests__/trigger.spec.ts index 787b9b4a..998478ff 100644 --- a/packages/cli/e2e/__tests__/trigger.spec.ts +++ b/packages/cli/e2e/__tests__/trigger.spec.ts @@ -10,21 +10,21 @@ import { runCheckly } from '../run-checkly' describe('trigger', () => { let fixt: FixtureSandbox const executionId = uuid.v4() + // The fixture config derives its logicalId from EXECUTION_ID, and + // runCheckly runs with extendEnv: false — every invocation must pass the + // variable or the config is invalid, which aborts the command. + const runOptions = { env: { EXECUTION_ID: executionId } } beforeAll(async () => { fixt = await FixtureSandbox.create({ source: path.join(__dirname, 'fixtures', 'trigger-project'), }) - await runCheckly(fixt, ['deploy', '--force'], { - env: { EXECUTION_ID: executionId }, - }) + await runCheckly(fixt, ['deploy', '--force'], runOptions) }, 180_000) afterAll(async () => { try { - await runCheckly(fixt, ['destroy', '--force'], { - env: { EXECUTION_ID: executionId }, - }) + await runCheckly(fixt, ['destroy', '--force'], runOptions) } catch { // cleanup best-effort } @@ -42,7 +42,7 @@ describe('trigger', () => { `production,backend,${executionId}`, '--tags', `production,frontend,${executionId}`, - ]) + ], runOptions) expect(stdout).toContain(secretEnv) expect(stdout).toContain('Prod Backend Check') @@ -56,7 +56,7 @@ describe('trigger', () => { 'trigger', '--tags', 'no-checks-match-this-tag', - ]) + ], runOptions) expect.unreachable('Expected command to fail') } catch (err) { if (err instanceof ExecaError) { @@ -75,7 +75,7 @@ describe('trigger', () => { '--tags', 'no-checks-match-this-tag', '--fail-on-no-matching', - ]) + ], runOptions) expect.unreachable('Expected command to fail') } catch (err) { if (err instanceof ExecaError) { @@ -93,7 +93,7 @@ describe('trigger', () => { '--tags', 'no-checks-match-this-tag', '--no-fail-on-no-matching', - ]) + ], runOptions) expect(stdout).toContain('No matching checks were found.') }) From 9c860629c9a137454280b3fe83d0661efba0793a Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 28 Aug 2026 04:38:09 +0900 Subject: [PATCH 5/5] feat(cli): report every nested config validation issue as its own diagnostic [RED-914] The runner.registries and bundle.packages.prune validators used to stop at their first problem, so a config with several mistakes inside one of those blocks surfaced them one edit-run cycle at a time. Both validators now walk the whole value, reporting each issue through a callback that the config loader turns into an individual diagnostic; the throwing entry points the bundler uses keep their contract by rethrowing the first issue. Cascading noise is gated rather than reported: an invalid or empty upstreams block skips the per-rule upstream-existence checks, an invalid final pattern skips the match-all requirement, and a match-all rule that merely sits in the wrong place is not additionally reported as missing. Pattern issues from the per-class prune map are prefixed with the dependency class so the same bad pattern in two classes stays distinguishable. Validation messages that feed diagnostics Reason: lines now end with terminal punctuation; this also touches the embedded-package spec errors and dependency cache version errors shared with other code paths. Co-Authored-By: Claude Fable 5 --- .../__tests__/checkly-config-loader.spec.ts | 34 +++ .../bundle-packages-prune-multiple-errors.js | 19 ++ .../runner-registries-multiple-errors.js | 23 ++ .../__tests__/package-prune.spec.ts | 51 +++- .../src/services/check-parser/cache-hash.ts | 4 +- .../services/check-parser/package-prune.ts | 122 +++++++-- .../cli/src/services/checkly-config-loader.ts | 26 +- .../src/services/embedded-packages/spec.ts | 4 +- .../runner/__tests__/registries.spec.ts | 116 ++++++++- .../cli/src/services/runner/registries.ts | 233 +++++++++++------- 10 files changed, 497 insertions(+), 135 deletions(-) create mode 100644 packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js create mode 100644 packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js diff --git a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts index d9af5fef..9e1e4352 100644 --- a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts +++ b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts @@ -275,6 +275,40 @@ describe('loadChecklyConfig()', () => { ['runner-registries-misspelled-key.js'], )).rejects.toThrow(`Property "runner.registires" is not supported`) }) + it('reports every runner.registries issue in a single run', async () => { + const error = await loadInvalidConfig('runner-registries-multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`property "runner.registries"`), + }), + expect.objectContaining({ + message: expect.stringContaining(`upstream 'yarnpkg': 'auth.type' must be 'bearer'`), + }), + expect.objectContaining({ + message: expect.stringContaining(`upstream 'yarnpkg': 'auth.token' must be exactly one environment variable reference`), + }), + expect.objectContaining({ + message: expect.stringContaining(`packages[0]: upstream 'red' is not defined under 'upstreams' (defined: 'yarnpkg')`), + }), + ])) + }) + it('reports every bundle.packages.prune issue in a single run', async () => { + const error = await loadInvalidConfig('bundle-packages-prune-multiple-errors.js') + expect(error.diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining(`property "bundle.packages.prune"`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'peerDependences' is not a dependency class`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'name@version' pins are not supported here`), + }), + expect.objectContaining({ + message: expect.stringContaining(`'dependencies' must be true or an array of package name patterns`), + }), + ])) + }) it('rejects a runner.registries rule using an unknown upstream name', async () => { await expect(loadChecklyConfig( configDir, diff --git a/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js new file mode 100644 index 00000000..c223f296 --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/bundle-packages-prune-multiple-errors.js @@ -0,0 +1,19 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// inside bundle.packages.prune to verify that all of them are reported in a +// single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + bundle: { + packages: { + prune: { + peerDependences: true, + devDependencies: ['ok-name', 'bad@1.0.0'], + dependencies: 5, + }, + }, + }, +} + +export default config diff --git a/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js b/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js new file mode 100644 index 00000000..d07ac1ec --- /dev/null +++ b/packages/cli/src/services/__tests__/fixtures/configs/runner-registries-multiple-errors.js @@ -0,0 +1,23 @@ +// Plain JS on purpose: bypasses the TypeScript type of ChecklyConfig so the +// runtime validation is what rejects it. Contains several independent errors +// inside runner.registries to verify that all of them are reported in a +// single run. +const config = { + projectName: 'test-config-project', + logicalId: 'test-config-project', + runner: { + registries: { + upstreams: { + yarnpkg: { + url: 'https://registry.yarnpkg.com/', + auth: {}, + }, + }, + packages: [ + { pattern: '**', upstreams: ['yarnpkg', 'red'] }, + ], + }, + }, +} + +export default config diff --git a/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts b/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts index 1fee83f4..4bc33ba5 100644 --- a/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts +++ b/packages/cli/src/services/check-parser/__tests__/package-prune.spec.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest' import { + collectPackagePruneIssues, DEPENDENCY_CLASSES, normalizePackagePrune, prunePackageJson, @@ -127,7 +128,55 @@ describe('normalizePackagePrune()', () => { it('rejects invalid patterns in either shape', () => { expect(() => normalizePackagePrune(['pkg@1.0.0'])).toThrow(InvalidPackageNamePatternError) expect(() => normalizePackagePrune({ dependencies: ['pkg@1.0.0'] })) - .toThrow(InvalidPackageNamePatternError) + .toThrow(`'dependencies': Invalid package name pattern 'pkg@1.0.0'`) + }) +}) + +function packagePruneIssueErrors (raw: unknown): Error[] { + const issues: Error[] = [] + collectPackagePruneIssues(raw as any, issue => issues.push(issue)) + return issues +} + +function packagePruneIssues (raw: unknown): string[] { + return packagePruneIssueErrors(raw).map(issue => issue.message) +} + +describe('collectPackagePruneIssues()', () => { + it('returns no issues for valid values', () => { + expect(packagePruneIssues(undefined)).toEqual([]) + expect(packagePruneIssues(['@acme/*'])).toEqual([]) + expect(packagePruneIssues({ peerDependencies: true })).toEqual([]) + }) + + it('collects every issue instead of stopping at the first', () => { + const issues = packagePruneIssues({ + peerDependences: true, + devDependencies: ['ok-name', 'bad@1.0.0'], + dependencies: 5, + }) + expect(issues).toEqual([ + expect.stringContaining(`'peerDependences' is not a dependency class`), + expect.stringContaining(`'name@version' pins are not supported here`), + expect.stringContaining(`'dependencies' must be true or an array of package name patterns`), + ]) + }) + + it('reports a bad array-shape entry once, not once per dependency class', () => { + const issues = packagePruneIssueErrors(['pkg@1.0.0']) + expect(issues).toHaveLength(1) + expect(issues[0]).toBeInstanceOf(InvalidPackageNamePatternError) + }) + + it('distinguishes the same bad pattern across dependency classes', () => { + const issues = packagePruneIssues({ + dependencies: ['bad@1.0.0'], + devDependencies: ['bad@1.0.0'], + }) + expect(issues).toEqual([ + expect.stringContaining(`'dependencies': Invalid package name pattern 'bad@1.0.0'`), + expect.stringContaining(`'devDependencies': Invalid package name pattern 'bad@1.0.0'`), + ]) }) }) diff --git a/packages/cli/src/services/check-parser/cache-hash.ts b/packages/cli/src/services/check-parser/cache-hash.ts index a01f4c87..6aa1b9f4 100644 --- a/packages/cli/src/services/check-parser/cache-hash.ts +++ b/packages/cli/src/services/check-parser/cache-hash.ts @@ -478,12 +478,12 @@ export function normalizeDependencyCacheVersion (version: string | number | unde } if (typeof version === 'number') { if (!Number.isSafeInteger(version)) { - throw new Error(`Dependency cache version must be a safe integer if given as a number, got ${version}`) + throw new Error(`Dependency cache version must be a safe integer if given as a number, got ${version}.`) } return String(version) } if (typeof version !== 'string') { - throw new Error(`Dependency cache version must be a string or a safe integer, got ${typeof version}`) + throw new Error(`Dependency cache version must be a string or a safe integer, got ${typeof version}.`) } return version } diff --git a/packages/cli/src/services/check-parser/package-prune.ts b/packages/cli/src/services/check-parser/package-prune.ts index 9ebc1179..75ebb6ad 100644 --- a/packages/cli/src/services/check-parser/package-prune.ts +++ b/packages/cli/src/services/check-parser/package-prune.ts @@ -36,43 +36,81 @@ export type NormalizedPackagePrune = { [K in DependencyClass]?: true | PackageNamePattern[] } -const SHAPE_ERROR = `must be an array of package name patterns or an object keyed by dependency class` +const SHAPE_ERROR = `must be an array of package name patterns or an object keyed by dependency class.` + +/** + * Walks a `bundle.packages.prune` value, reporting every issue through + * `onIssue` so that all of them surface in a single run — the config + * loader turns each one into its own diagnostic. Pattern issues from the + * per-class map shape are prefixed with the dependency class; array-shape + * pattern issues keep their `InvalidPackageNamePatternError` class and + * message as-is. + */ +export function collectPackagePruneIssues ( + raw: BundlePackagesPrune | undefined, + onIssue: (issue: Error) => void, +): void { + normalizePackagePruneInternal(raw, onIssue) +} /** * Validates and normalizes a `bundle.packages.prune` value. Returns * `undefined` when there is nothing to do — the value is absent, or every - * shape it carries is empty. Throws `InvalidPackageNamePatternError` on an - * invalid pattern and a plain `Error` on an invalid shape; the config - * loader relies on that to reject plain-JS configs that bypass the - * TypeScript type. + * shape it carries is empty. Throws the first issue found; the bundler + * relies on that to reject a value that bypassed config validation. */ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): NormalizedPackagePrune | undefined { + let firstIssue: Error | undefined + const normalized = normalizePackagePruneInternal(raw, issue => { + firstIssue ??= issue + }) + if (firstIssue !== undefined) { + throw firstIssue + } + + return normalized +} + +function normalizePackagePruneInternal ( + raw: BundlePackagesPrune | undefined, + onIssue: (issue: Error) => void, +): NormalizedPackagePrune | undefined { if (raw === undefined) { return undefined } - let perClass: Record if (Array.isArray(raw)) { - perClass = Object.fromEntries(DEPENDENCY_CLASSES.map(dependencyClass => [dependencyClass, raw])) - } else if (raw !== null && typeof raw === 'object') { - // A non-plain object (a Set, a Map, a Date) has no own enumerable - // string keys, so without this check it would silently normalize to - // "nothing to do" instead of being rejected. - const proto = Object.getPrototypeOf(raw) - if (proto !== Object.prototype && proto !== null) { - throw new Error(SHAPE_ERROR) + // The pattern array applies to every dependency class, but is parsed + // only once so a bad entry does not repeat as an issue for each class. + const patterns = parsePatternArray(raw, onIssue) + if (patterns === undefined) { + return undefined } - perClass = raw - } else { - throw new Error(SHAPE_ERROR) + // A separate array per class, so a consumer mutating one class's list + // cannot silently change the others. (The pattern objects themselves + // are shared; they are read-only everywhere.) + return Object.fromEntries( + DEPENDENCY_CLASSES.map(dependencyClass => [dependencyClass, [...patterns]]), + ) as NormalizedPackagePrune + } + + // A non-plain object (a Set, a Map, a Date) has no own enumerable + // string keys, so without the prototype check it would silently + // normalize to "nothing to do" instead of being rejected. + const proto = raw !== null && typeof raw === 'object' ? Object.getPrototypeOf(raw) : undefined + if (proto !== Object.prototype && proto !== null) { + onIssue(new Error(SHAPE_ERROR)) + return undefined } + const perClass: Record = raw const normalized: NormalizedPackagePrune = {} for (const [key, value] of Object.entries(perClass)) { if (!DEPENDENCY_CLASSES.includes(key as DependencyClass)) { - throw new Error( - `'${key}' is not a dependency class (expected one of ${DEPENDENCY_CLASSES.join(', ')})`, - ) + onIssue(new Error( + `'${key}' is not a dependency class (expected one of ${DEPENDENCY_CLASSES.join(', ')}).`, + )) + continue } if (value === undefined) { continue @@ -82,16 +120,16 @@ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): No continue } if (Array.isArray(value)) { - if (value.length === 0) { - continue + // The dependency class prefixes any pattern issue: the same bad + // pattern may appear under several classes, and without the prefix + // their issues would be indistinguishable. + const patterns = parsePatternArray(value, onIssue, `'${key}'`) + if (patterns !== undefined) { + normalized[key as DependencyClass] = patterns } - // Parsed per class even for the array shape, so no two classes share - // one pattern array instance and a consumer mutating one cannot - // silently change the others. - normalized[key as DependencyClass] = value.map(entry => parsePackageNamePattern(entry as string)) continue } - throw new Error(`'${key}' must be true or an array of package name patterns`) + onIssue(new Error(`'${key}' must be true or an array of package name patterns.`)) } if (Object.keys(normalized).length === 0) { @@ -101,6 +139,36 @@ export function normalizePackagePrune (raw: BundlePackagesPrune | undefined): No return normalized } +/** + * Parses a pattern array, collecting an issue per invalid entry + * (prefixed with `context` when given). Returns `undefined` when the + * array is empty (nothing to do) or any entry is invalid. + */ +function parsePatternArray ( + value: unknown[], + onIssue: (issue: Error) => void, + context?: string, +): PackageNamePattern[] | undefined { + const patterns: PackageNamePattern[] = [] + for (const entry of value) { + try { + patterns.push(parsePackageNamePattern(entry as string)) + } catch (cause) { + onIssue(context === undefined + ? cause as Error + : new Error(`${context}: ${(cause as Error).message}`, { cause })) + } + } + + // Each entry contributed either a pattern or an issue, so a shorter + // patterns list means some entry was invalid. + if (patterns.length !== value.length || patterns.length === 0) { + return undefined + } + + return patterns +} + export interface PrunePackageJsonResult { content: string /** diff --git a/packages/cli/src/services/checkly-config-loader.ts b/packages/cli/src/services/checkly-config-loader.ts index da532b7b..dc0295fd 100644 --- a/packages/cli/src/services/checkly-config-loader.ts +++ b/packages/cli/src/services/checkly-config-loader.ts @@ -17,9 +17,9 @@ import { ReporterType } from '../reporters/reporter.js' import { PlaywrightConfig } from '../constructs/playwright-config.js' import { FileLoader } from '../loader/index.js' import { normalizeDependencyCacheVersion } from './check-parser/cache-hash.js' -import { BundlePackagesPrune, normalizePackagePrune } from './check-parser/package-prune.js' +import { BundlePackagesPrune, collectPackagePruneIssues } from './check-parser/package-prune.js' import { parseEmbeddedPackageSpec } from './embedded-packages/spec.js' -import { Registries, validateRegistries } from './runner/registries.js' +import { Registries, collectRegistriesIssues } from './runner/registries.js' export type CheckConfigDefaults = Pick { + diagnostics.add(new InvalidPropertyValueDiagnostic('bundle.packages.prune', issue)) + }) const embeddedPackages = packages.embed if (embeddedPackages === undefined) { @@ -610,12 +605,7 @@ function validateRunner (config: ChecklyConfig, diagnostics: Diagnostics): void return } - try { - validateRegistries(registries) - } catch (cause) { - diagnostics.add(new InvalidPropertyValueDiagnostic( - 'runner.registries', - cause as Error, - )) - } + collectRegistriesIssues(registries, issue => { + diagnostics.add(new InvalidPropertyValueDiagnostic('runner.registries', issue)) + }) } diff --git a/packages/cli/src/services/embedded-packages/spec.ts b/packages/cli/src/services/embedded-packages/spec.ts index 4347fffb..f0701c5e 100644 --- a/packages/cli/src/services/embedded-packages/spec.ts +++ b/packages/cli/src/services/embedded-packages/spec.ts @@ -203,7 +203,7 @@ export class InvalidEmbeddedPackageSpecError extends Error { readonly reason: string constructor (spec: string, reason: string) { - super(`Invalid embedded package '${spec}': ${reason}`) + super(`Invalid embedded package '${spec}': ${reason}.`) this.name = 'InvalidEmbeddedPackageSpecError' this.reason = reason } @@ -293,7 +293,7 @@ export type PackageNamePattern = Pick ({ upstreams: { @@ -16,6 +16,118 @@ const validRegistries = (): Registries => ({ ], }) +function registriesIssues (value: unknown): string[] { + const issues: string[] = [] + collectRegistriesIssues(value, issue => issues.push(issue.message)) + return issues +} + +describe('collectRegistriesIssues()', () => { + it('returns no issues for a valid configuration', () => { + expect(registriesIssues(validRegistries())).toEqual([]) + }) + + it('collects every issue instead of stopping at the first', () => { + const value: any = validRegistries() + value.upstreams.internal.auth = {} + value.packages[0].upstreams = ['internal', 'mirror'] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`upstream 'internal': 'auth.type' must be 'bearer'`), + expect.stringContaining(`upstream 'internal': 'auth.token' must be exactly one environment variable reference`), + expect.stringContaining(`packages[0]: upstream 'mirror' is not defined under 'upstreams'`), + ]) + }) + + it('skips upstream-existence checks when the upstreams block is invalid', () => { + const value: any = validRegistries() + value.upstreams = 'nope' + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must be an object mapping upstream names to { url, auth? }`), + ]) + }) + + it('skips upstream-existence checks when the upstreams block is empty', () => { + const value: any = validRegistries() + value.upstreams = {} + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must define at least one upstream`), + ]) + }) + + it('skips the match-all-position check when the final pattern is invalid', () => { + const value: any = validRegistries() + value.packages[1].pattern = 42 + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[1]: 'pattern' must be a string`), + ]) + }) + + it('does not ask for a match-all rule when a misplaced one exists', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '**', upstreams: ['npmjs'] }, + { pattern: '@acme/**', upstreams: ['internal'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: rules after a '**' match-all rule can never apply`), + ]) + }) + + it('does not ask for a match-all rule when the only rule is an exclusion', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '!**', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: exclusion patterns ('!') are not supported in routing rules`), + ]) + }) + + it('reports non-string upstream entries even when the upstreams block is invalid', () => { + const value: any = validRegistries() + value.upstreams = {} + value.packages = [ + { pattern: '**', upstreams: ['npmjs', 42] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`'upstreams' must define at least one upstream`), + expect.stringContaining(`packages[0]: 'upstreams'[1] must be an upstream name`), + ]) + }) + + it('does not flag a non-final excluded match-all as a misplaced match-all rule', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '!**', upstreams: ['npmjs'] }, + { pattern: '**', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[0]: exclusion patterns ('!') are not supported in routing rules`), + ]) + }) + + it('still asks for a match-all rule when the trailing exclusion is not one', () => { + const value: any = validRegistries() + value.packages = [ + { pattern: '@acme/**', upstreams: ['npmjs'] }, + { pattern: '!@acme/foo', upstreams: ['npmjs'] }, + ] + const issues = registriesIssues(value) + expect(issues).toEqual([ + expect.stringContaining(`packages[1]: exclusion patterns ('!') are not supported in routing rules`), + expect.stringContaining(`'packages' must end with a match-all rule`), + ]) + }) +}) + describe('validateRegistries()', () => { it('accepts a valid configuration', () => { const value = validRegistries() @@ -95,7 +207,7 @@ describe('validateRegistries()', () => { it('rejects unknown fields at every level', () => { const top = { ...validRegistries(), extra: true } expect(() => validateRegistries(top)) - .toThrow(`'runner.registries': unknown field 'extra' (expected only: 'upstreams', 'packages')`) + .toThrow(`unknown field 'extra' (expected only: 'upstreams', 'packages')`) const upstream = validRegistries() ;(upstream.upstreams.internal as any).authh = { type: 'bearer', token: '${T}' } diff --git a/packages/cli/src/services/runner/registries.ts b/packages/cli/src/services/runner/registries.ts index b3e3449d..b8d8d103 100644 --- a/packages/cli/src/services/runner/registries.ts +++ b/packages/cli/src/services/runner/registries.ts @@ -1,4 +1,4 @@ -import { parsePackageNamePattern } from '../embedded-packages/spec.js' +import { PackageNamePattern, parsePackageNamePattern } from '../embedded-packages/spec.js' import { COMPOSABLE_URL_REQUIREMENT, parseComposableUrl } from '../embedded-packages/url.js' /** @@ -154,108 +154,156 @@ function isPlainObject (value: unknown): value is Record { * an intention, and ignoring it silently disables whatever the user meant * to configure, surfacing only as an install failure on the runner. */ -function rejectUnknownKeys (context: string, value: Record, known: string[]): void { +function collectUnknownKeyIssues ( + onIssue: (issue: Error) => void, + context: string | undefined, + value: Record, + known: string[], +): void { + const prefix = context === undefined ? '' : `${context}: ` for (const key of Object.keys(value)) { if (!known.includes(key)) { - throw new Error(`${context}: unknown field '${key}' (expected only: ${known.map(k => `'${k}'`).join(', ')})`) + onIssue(new Error(`${prefix}unknown field '${key}' (expected only: ${known.map(k => `'${k}'`).join(', ')}).`)) } } } /** - * Validates the value of `runner.registries` and returns it typed. All - * failures throw a plain `Error` whose message names the offending part - * relative to `runner.registries`; the config loader wraps it with the - * full config path. Invalid URLs and tokens are deliberately never echoed - * back — a malformed registry URL may carry an inline credential. + * Walks a registries value, reporting every issue through `onIssue` so + * that all of them surface in a single run — the config loader turns each + * one into its own diagnostic. Messages name the offending part relative + * to the registries block itself. Invalid URLs and tokens are + * deliberately never echoed back — a malformed registry URL may carry an + * inline credential. + * + * Structural problems gate their dependents: an `upstreams` block that is + * not an object (or is empty) skips the per-rule upstream-existence + * checks (they would all fail spuriously), and a rule whose pattern is + * invalid skips the match-all checks for that rule. * * Plain-JS configs bypass the TypeScript type, so the shape must be * enforced at runtime, exactly as `validateBundle` does for * `bundle.packages`. */ -export function validateRegistries (value: unknown): Registries { +export function collectRegistriesIssues (value: unknown, onIssue: (issue: Error) => void): void { if (!isPlainObject(value)) { - throw new Error(`must be an object`) + onIssue(new Error(`must be an object.`)) + return } - rejectUnknownKeys(`'runner.registries'`, value, ['upstreams', 'packages']) + // No context: the diagnostic wrapping the issue already names the + // registries block's own config path. + collectUnknownKeyIssues(onIssue, undefined, value, ['upstreams', 'packages']) const { upstreams, packages } = value + let upstreamNames: string[] | undefined if (!isPlainObject(upstreams)) { - throw new Error(`'upstreams' must be an object mapping upstream names to { url, auth? }`) - } - - const upstreamNames = Object.keys(upstreams) - if (upstreamNames.length === 0) { - throw new Error(`'upstreams' must define at least one upstream`) - } - - for (const name of upstreamNames) { - validateUpstream(name, upstreams[name]) + onIssue(new Error(`'upstreams' must be an object mapping upstream names to { url, auth? }.`)) + } else if (Object.keys(upstreams).length === 0) { + // Gates the per-rule upstream-existence checks the same way a + // non-object block does: with no upstreams defined, every rule would + // otherwise cascade a spurious issue of its own. + onIssue(new Error(`'upstreams' must define at least one upstream.`)) + } else { + upstreamNames = Object.keys(upstreams) + for (const name of upstreamNames) { + collectUpstreamIssues(onIssue, name, upstreams[name]) + } } if (!Array.isArray(packages)) { - throw new Error(`'packages' must be an array of { pattern, upstreams } routing rules`) + onIssue(new Error(`'packages' must be an array of { pattern, upstreams } routing rules.`)) + return } - let lastPattern: string | undefined + let lastPattern: PackageNamePattern | undefined + let sawMatchAll = false for (const [index, rule] of packages.entries()) { - const { pattern } = validatePackageRoutingRule(index, rule, upstreamNames) + const pattern = collectPackageRoutingRuleIssues(onIssue, index, rule, upstreamNames) // First match wins, so nothing past a match-all rule could ever // apply. Requiring the match-all to be the final rule both guarantees // every package a route and keeps silently dead rules out of the // config. The final-rule check below also rejects an empty rule list. - if (pattern === MATCH_ALL_PATTERN && index !== packages.length - 1) { - throw new Error( - `packages[${index}]: rules after a '${MATCH_ALL_PATTERN}' match-all rule can never apply ` - + `(the first matching rule wins); move the match-all rule last`, - ) + // An excluded '!**' — invalid in itself — is not a match-all rule, so + // it never triggers the position issue, and it only satisfies the + // final-rule check when it is itself the final rule (where removing + // the '!' is all it takes; anywhere else the rule list still needs a + // trailing match-all). + if (pattern !== undefined && pattern.name === MATCH_ALL_PATTERN) { + if (!pattern.exclude && index !== packages.length - 1) { + onIssue(new Error( + `packages[${index}]: rules after a '${MATCH_ALL_PATTERN}' match-all rule can never apply ` + + `(the first matching rule wins); move the match-all rule last.`, + )) + } + if (!pattern.exclude || index === packages.length - 1) { + sawMatchAll = true + } } lastPattern = pattern } - if (lastPattern !== MATCH_ALL_PATTERN) { - throw new Error( + // Skipped when a match-all rule exists (a misplaced one already carries + // its own issue telling the user to move it, and asking them to add one + // they have would contradict it), and when the final rule's own pattern + // is invalid (its issue is already reported and this check would be + // speculative). + if (!sawMatchAll && (packages.length === 0 || lastPattern !== undefined)) { + onIssue(new Error( `'packages' must end with a match-all rule ({ pattern: '${MATCH_ALL_PATTERN}', ... }) ` - + `so that every package has a route`, - ) + + `so that every package has a route.`, + )) + } +} + +/** + * Validates the value of `runner.registries` and returns it typed, + * throwing the first issue found. Invalid URLs and tokens are never + * echoed back — a malformed registry URL may carry an inline credential. + */ +export function validateRegistries (value: unknown): Registries { + let firstIssue: Error | undefined + collectRegistriesIssues(value, issue => { + firstIssue ??= issue + }) + if (firstIssue !== undefined) { + throw firstIssue } return value as unknown as Registries } -function validateUpstream (name: string, value: unknown): void { +function collectUpstreamIssues (onIssue: (issue: Error) => void, name: string, value: unknown): void { if (!UPSTREAM_NAME_RE.test(name)) { - throw new Error( + onIssue(new Error( `upstream name '${name}' is invalid: names must start with a letter or digit ` - + `and contain only letters, digits, '-' and '_'`, - ) + + `and contain only letters, digits, '-' and '_'.`, + )) } if (!isPlainObject(value)) { - throw new Error(`upstream '${name}' must be an object with a 'url'`) + onIssue(new Error(`upstream '${name}' must be an object with a 'url'.`)) + return } - rejectUnknownKeys(`upstream '${name}'`, value, ['url', 'auth']) + collectUnknownKeyIssues(onIssue, `upstream '${name}'`, value, ['url', 'auth']) const { url, auth } = value const parsedUrl = typeof url === 'string' ? parseComposableUrl(url) : undefined if (parsedUrl === undefined) { - throw new Error(`upstream '${name}': 'url' must be ${COMPOSABLE_URL_REQUIREMENT}`) - } - - // An inline credential would ship in plaintext inside the uploaded code - // bundle — the exact outcome the ${VAR}-only rule on auth tokens exists - // to prevent. The URL is not echoed for the same reason. - if (parsedUrl.username !== '' || parsedUrl.password !== '') { - throw new Error( + onIssue(new Error(`upstream '${name}': 'url' must be ${COMPOSABLE_URL_REQUIREMENT}.`)) + } else if (parsedUrl.username !== '' || parsedUrl.password !== '') { + // An inline credential would ship in plaintext inside the uploaded code + // bundle — the exact outcome the ${VAR}-only rule on auth tokens exists + // to prevent. The URL is not echoed for the same reason. + onIssue(new Error( `upstream '${name}': 'url' must not contain credentials; ` - + `use auth: { type: 'bearer', token: '\${VAR}' } instead`, - ) + + `use auth: { type: 'bearer', token: '\${VAR}' } instead.`, + )) } if (auth === undefined) { @@ -263,65 +311,84 @@ function validateUpstream (name: string, value: unknown): void { } if (!isPlainObject(auth)) { - throw new Error(`upstream '${name}': 'auth' must be an object if set`) + onIssue(new Error(`upstream '${name}': 'auth' must be an object if set.`)) + return } - rejectUnknownKeys(`upstream '${name}': 'auth'`, auth, ['type', 'token']) + collectUnknownKeyIssues(onIssue, `upstream '${name}': 'auth'`, auth, ['type', 'token']) if (auth.type !== 'bearer') { - throw new Error(`upstream '${name}': 'auth.type' must be 'bearer'`) + onIssue(new Error(`upstream '${name}': 'auth.type' must be 'bearer'.`)) } if (typeof auth.token !== 'string' || !ENV_VAR_REFERENCE_RE.test(auth.token)) { - throw new Error( + onIssue(new Error( `upstream '${name}': 'auth.token' must be exactly one environment variable reference in \${VAR} syntax ` + `(e.g. '\${NPM_TOKEN}'). The variable is resolved from the check's environment variables on the ` - + `Checkly runner; any literal content would bake a secret into the code bundle`, - ) + + `Checkly runner; any literal content would bake a secret into the code bundle.`, + )) } } -function validatePackageRoutingRule (index: number, value: unknown, upstreamNames: string[]): PackageRoutingRule { +/** + * Collects the issues of one routing rule. Returns the rule's parsed + * pattern (its name has any leading '!' stripped) when it is a parseable + * string, so the caller can run the match-all checks; `undefined` means + * the pattern is unknown and those checks do not apply. When + * `upstreamNames` is undefined (the `upstreams` block itself is invalid), + * the upstream-existence checks are skipped. + */ +function collectPackageRoutingRuleIssues ( + onIssue: (issue: Error) => void, + index: number, + value: unknown, + upstreamNames: string[] | undefined, +): PackageNamePattern | undefined { if (!isPlainObject(value)) { - throw new Error(`packages[${index}] must be an object with 'pattern' and 'upstreams'`) + onIssue(new Error(`packages[${index}] must be an object with 'pattern' and 'upstreams'.`)) + return undefined } - rejectUnknownKeys(`packages[${index}]`, value, ['pattern', 'upstreams']) + collectUnknownKeyIssues(onIssue, `packages[${index}]`, value, ['pattern', 'upstreams']) const { pattern, upstreams } = value + let knownPattern: PackageNamePattern | undefined if (typeof pattern !== 'string') { - throw new Error(`packages[${index}]: 'pattern' must be a string`) - } - - let parsed - try { - parsed = parsePackageNamePattern(pattern) - } catch (cause) { - throw new Error(`packages[${index}]: ${(cause as Error).message}`, { cause }) - } - - if (parsed.exclude) { - throw new Error( - `packages[${index}]: exclusion patterns ('!') are not supported in routing rules; ` - + `each rule stands alone, so there is nothing for an exclusion to subtract from`, - ) + onIssue(new Error(`packages[${index}]: 'pattern' must be a string.`)) + } else { + try { + const parsed = parsePackageNamePattern(pattern) + if (parsed.exclude) { + onIssue(new Error( + `packages[${index}]: exclusion patterns ('!') are not supported in routing rules; ` + + `each rule stands alone, so there is nothing for an exclusion to subtract from.`, + )) + } + knownPattern = parsed + } catch (cause) { + onIssue(new Error(`packages[${index}]: ${(cause as Error).message}`, { cause })) + } } if (!Array.isArray(upstreams) || upstreams.length === 0) { - throw new Error(`packages[${index}]: 'upstreams' must be a non-empty array of upstream names`) - } - - for (const name of upstreams) { - if (typeof name !== 'string' || !upstreamNames.includes(name)) { - throw new Error( - `packages[${index}]: upstream '${String(name)}' is not defined under 'upstreams' ` - + `(defined: ${upstreamNames.map(known => `'${known}'`).join(', ')})`, - ) + onIssue(new Error(`packages[${index}]: 'upstreams' must be a non-empty array of upstream names.`)) + } else { + for (const [entryIndex, name] of upstreams.entries()) { + if (typeof name !== 'string') { + // Unlike the existence check below, this does not depend on the + // 'upstreams' block, so it is never gated. + onIssue(new Error(`packages[${index}]: 'upstreams'[${entryIndex}] must be an upstream name.`)) + } else if (upstreamNames !== undefined && !upstreamNames.includes(name)) { + onIssue(new Error( + `packages[${index}]: upstream '${name}' is not defined under 'upstreams' ` + + `(defined: ${upstreamNames.map(known => `'${known}'`).join(', ')}).`, + )) + } } } - return value as unknown as PackageRoutingRule + return knownPattern } /**