From d8f64a8c8265d0aa37d88430a8e6806289fd4016 Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 19 Jun 2026 08:58:38 +0900 Subject: [PATCH 1/3] feat(cli): add early diagnostics to Construct [RED-641] Constructs validate via async validate(), but are built through a sync constructor that cannot do async work and by convention does not throw for user-facing validation. Add an earlyDiagnostics collector on the base Construct that a constructor can record issues into; base validate() merges it into the caller's Diagnostics, so the issues flow through the existing per-construct collection and reporting path with no changes to subclasses or commands. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01QBbrKMRZw7DjdqjSgnQX41 --- .../__tests__/early-diagnostics.spec.ts | 77 +++++++++++++++++++ packages/cli/src/constructs/construct.ts | 9 +++ 2 files changed, 86 insertions(+) create mode 100644 packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts diff --git a/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts b/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts new file mode 100644 index 000000000..aa47a3959 --- /dev/null +++ b/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts @@ -0,0 +1,77 @@ +import { describe, it, expect, beforeEach } from 'vitest' + +import { Session } from '../session.js' +import { Construct } from '../construct.js' +import { Diagnostics, ErrorDiagnostic } from '../diagnostics.js' +import { ConstructDiagnostics } from '../construct-diagnostics.js' + +class EarlyConstruct extends Construct { + constructor (logicalId: string) { + super('test', logicalId) + this.earlyDiagnostics.add(new ErrorDiagnostic({ + title: 'Early problem', + message: 'Something the constructor noticed.', + error: new Error('Something the constructor noticed.'), + })) + } + + describe (): string { + return `Test:${this.logicalId}` + } + + synthesize () { + return null + } +} + +describe('Construct early diagnostics', () => { + beforeEach(() => { + Session.reset() + Session.project = { addResource: () => {} } as any + }) + + it('surfaces diagnostics recorded in the constructor via validate()', async () => { + const construct = new EarlyConstruct('my-check') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('Something the constructor noticed.'), + }), + ])) + }) + + it('attributes early diagnostics to the construct when validated via ConstructDiagnostics', async () => { + const construct = new EarlyConstruct('my-check') + const diagnostics = new ConstructDiagnostics(construct) + await construct.validate(diagnostics) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('[Test:my-check]'), + }), + ])) + }) + + it('does not record any diagnostics when the constructor adds none', async () => { + class QuietConstruct extends Construct { + constructor (logicalId: string) { + super('test', logicalId) + } + + describe (): string { + return `Quiet:${this.logicalId}` + } + + synthesize () { + return null + } + } + + const construct = new QuietConstruct('my-check') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toHaveLength(0) + }) +}) diff --git a/packages/cli/src/constructs/construct.ts b/packages/cli/src/constructs/construct.ts index b96410371..4dfbb08c0 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -57,6 +57,13 @@ export abstract class Construct implements Validate, Bundle { member: boolean /** Absolute path to the check file that created this construct */ checkFileAbsolutePath?: string + /** + * Diagnostics recorded during construction. A constructor cannot perform the + * async work that validate() can, so any issue it notices (e.g. an argument + * of the wrong type) can be added here and is merged into the caller's + * Diagnostics by validate(). + */ + readonly earlyDiagnostics = new Diagnostics() /** * Creates a new construct instance. @@ -128,6 +135,8 @@ export abstract class Construct implements Validate, Bundle { */ // eslint-disable-next-line require-await async validate (diagnostics: Diagnostics): Promise { + diagnostics.extend(this.earlyDiagnostics) + if (!LOGICAL_ID_PATTERN.test(this.logicalId)) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'logicalId', From 35a7fa6c21a136cc2fd1c939d2e816a7a62ccc54 Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 19 Jun 2026 09:08:49 +0900 Subject: [PATCH 2/3] feat(cli): report invalid and duplicate logicalId as diagnostics [RED-641] Turn two construction-time hard errors into soft validation diagnostics via the new earlyDiagnostics mechanism, so they're collected and reported alongside other validation issues instead of aborting with an exception: - A non-string logicalId is recorded as a diagnostic in the Construct constructor and the value is coerced to a string; the matching throw in Session.validateCreateConstruct is removed. - A duplicate logicalId is recorded as a diagnostic on the Project in addResource instead of throwing. The duplicate resource is intentionally not stored, so the diagnostic is attributed to the project to ensure it surfaces during validation. Removes the now-unused ValidationError class. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01QBbrKMRZw7DjdqjSgnQX41 --- packages/cli/e2e/__tests__/test.spec.ts | 6 ++- .../__tests__/alert-channel.spec.ts | 21 +++++--- .../constructs/__tests__/check-group.spec.ts | 52 ++++++++++++------- .../__tests__/private-location.spec.ts | 29 +++++++---- .../src/constructs/__tests__/project.spec.ts | 35 ++++++++++++- .../__tests__/status-page-service.spec.ts | 26 ++++++---- packages/cli/src/constructs/construct.ts | 7 +++ packages/cli/src/constructs/project.ts | 16 +++++- packages/cli/src/constructs/session.ts | 5 -- .../cli/src/constructs/validator-error.ts | 1 - 10 files changed, 142 insertions(+), 56 deletions(-) delete mode 100644 packages/cli/src/constructs/validator-error.ts diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 284f2410c..0305ff2a7 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -174,8 +174,10 @@ describe('test', { timeout: 45000 }, () => { await runTest(fixt, []) } catch (err) { if (err instanceof ExecaError) { - expect((err.stderr as unknown as string).replace(/(\n {4})/gm, '')) - .toContain('Error: Resource of type \'check-group\' with logical id \'my-check-group\' already exists.') + // A duplicate logicalId is now reported as a fatal validation + // diagnostic (printed to stdout) rather than a thrown error. + expect((err.stdout as unknown as string).replace(/(\n {4})/gm, '')) + .toContain('A check-group with logicalId "my-check-group" already exists.') expect(err.exitCode).toBe(1) } else { throw err diff --git a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts index efbbfcb45..5a7818533 100644 --- a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts +++ b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts @@ -19,19 +19,24 @@ class TestAlertChannel extends AlertChannel { } describe('AlertChannel', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new TestAlertChannel('foo', { - }) - } + new TestAlertChannel('foo', {}) + new TestAlertChannel('foo', {}) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/check-group.spec.ts b/packages/cli/src/constructs/__tests__/check-group.spec.ts index 5ee4bfef9..9ffa667f1 100644 --- a/packages/cli/src/constructs/__tests__/check-group.spec.ts +++ b/packages/cli/src/constructs/__tests__/check-group.spec.ts @@ -11,20 +11,28 @@ describe('CheckGroup', () => { expect(CheckGroupV1).toBe(CheckGroup) }) - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV1('foo', { - name: 'Test', - }) - } + new CheckGroupV1('foo', { + name: 'Test', + }) + new CheckGroupV1('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { @@ -84,20 +92,28 @@ describe('CheckGroup', () => { }) describe('v2', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV2('foo', { - name: 'Test', - }) - } + new CheckGroupV2('foo', { + name: 'Test', + }) + new CheckGroupV2('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/private-location.spec.ts b/packages/cli/src/constructs/__tests__/private-location.spec.ts index 3be899e49..ca69dbbab 100644 --- a/packages/cli/src/constructs/__tests__/private-location.spec.ts +++ b/packages/cli/src/constructs/__tests__/private-location.spec.ts @@ -5,21 +5,30 @@ import { Project } from '../project.js' import { Session } from '../session.js' describe('PrivateLocation', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new PrivateLocation('foo', { - name: 'Test', - slugName: 'test', - }) - } + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/project.spec.ts b/packages/cli/src/constructs/__tests__/project.spec.ts index 3e5d1c035..47f38afa5 100644 --- a/packages/cli/src/constructs/__tests__/project.spec.ts +++ b/packages/cli/src/constructs/__tests__/project.spec.ts @@ -1,11 +1,12 @@ import { describe, it, expect, beforeEach } from 'vitest' +import { Project } from '../project.js' import { Session } from '../session.js' import { Construct } from '../construct.js' import { Diagnostics } from '../diagnostics.js' class TestConstruct extends Construct { - constructor (logicalId: string) { + constructor (logicalId: any) { super('test', logicalId) } @@ -74,4 +75,36 @@ describe('Construct logicalId validation', () => { }), ])) }) + + it('should produce a diagnostic for a non-string logicalId instead of throwing', async () => { + const construct = new TestConstruct(123) + expect(construct.logicalId).toBe('123') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('Expected a string but received type "number"'), + }), + ])) + }) + + it('should produce a diagnostic for a duplicate logicalId instead of throwing', async () => { + Session.reset() + const project = new Project('test-project', { name: 'Test' }) + Session.project = project + + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('[Test:duplicate-id]'), + message: expect.stringContaining('A check with logicalId "duplicate-id" already exists'), + }), + ])) + }) }) diff --git a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts index 11f97ab26..aaa7bd8dd 100644 --- a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts +++ b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts @@ -5,20 +5,28 @@ import { Project } from '../project.js' import { Session } from '../session.js' describe('StatusPageService', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new StatusPageService('foo', { - name: 'foo', - }) - } + new StatusPageService('foo', { + name: 'foo', + }) + new StatusPageService('foo', { + name: 'foo', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/construct.ts b/packages/cli/src/constructs/construct.ts index 4dfbb08c0..58016f329 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -74,6 +74,13 @@ export abstract class Construct implements Validate, Bundle { * @param member Whether this construct is a member of the project */ constructor (type: string, logicalId: string, physicalId?: string | number, member?: boolean) { + if (typeof logicalId !== 'string') { + this.earlyDiagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`Expected a string but received type "${typeof logicalId}".`), + )) + logicalId = String(logicalId) + } this.logicalId = logicalId this.type = type this.physicalId = physicalId diff --git a/packages/cli/src/constructs/project.ts b/packages/cli/src/constructs/project.ts index d2be3dfc0..271db5335 100644 --- a/packages/cli/src/constructs/project.ts +++ b/packages/cli/src/constructs/project.ts @@ -9,7 +9,7 @@ import { StatusPage, StatusPageService, } from './/index.js' import { Diagnostics } from './diagnostics.js' -import { ConstructDiagnostics, InvalidPropertyValueDiagnostic } from './construct-diagnostics.js' +import { ConstructDiagnostic, ConstructDiagnostics, InvalidPropertyValueDiagnostic } from './construct-diagnostics.js' import { ProjectBundle, ProjectDataBundle } from './project-bundle.js' import { Bundler } from '../services/check-parser/bundler.js' import { Session } from './session.js' @@ -126,7 +126,19 @@ export class Project extends Construct { return } - throw new Error(`Resource of type '${type}' with logical id '${logicalId}' already exists.`) + // The duplicate resource is intentionally not stored, so it is never + // visited by the validate() fan-out over project data. Record the + // diagnostic on the project itself so it surfaces during validation, + // wrapping it in a ConstructDiagnostic so the output names the + // offending construct. + this.earlyDiagnostics.add(new ConstructDiagnostic( + resource, + new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`A ${type} with logicalId "${logicalId}" already exists.`), + ), + )) + return } this.data[type as keyof ProjectData][logicalId] = resource diff --git a/packages/cli/src/constructs/session.ts b/packages/cli/src/constructs/session.ts index 43912f408..94bc6a398 100644 --- a/packages/cli/src/constructs/session.ts +++ b/packages/cli/src/constructs/session.ts @@ -5,7 +5,6 @@ import { CheckConfigDefaults } from '../services/checkly-config-loader.js' import { type EngineDetectionResult } from '../services/engine-detector.js' import { Parser } from '../services/check-parser/parser.js' import { Construct } from './construct.js' -import { ValidationError } from './validator-error.js' import { PrivateLocationApi } from '../rest/private-locations.js' import { FileLoader, @@ -158,10 +157,6 @@ export class Session { } static validateCreateConstruct (construct: Construct) { - if (typeof construct.logicalId !== 'string') { - throw new ValidationError(`The "logicalId" of a ${construct.type} construct must be a string (logicalId=${construct.logicalId} [${typeof construct.logicalId}])`) - } - if (construct.type === PROJECT_CONSTRUCT_TYPE) { // Creating the construct is allowed - We're creating the project. } else if (Session.project) { diff --git a/packages/cli/src/constructs/validator-error.ts b/packages/cli/src/constructs/validator-error.ts deleted file mode 100644 index 9595a161f..000000000 --- a/packages/cli/src/constructs/validator-error.ts +++ /dev/null @@ -1 +0,0 @@ -export class ValidationError extends Error {} From 3478e40c7af660428e62e4d83298fabab184fc9c Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Fri, 19 Jun 2026 16:04:07 +0900 Subject: [PATCH 3/3] ci: run windows jobs on blacksmith-4vcpu-windows-2025 [temporary] Temporary: trial Blacksmith Windows runners to work around the GitHub windows-latest fleet slowdown that pushes the e2e suite past its timeouts. Intended to be dropped before merge. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01QBbrKMRZw7DjdqjSgnQX41 --- .github/workflows/test.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 523785232..ab187ad9e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -37,7 +37,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: test - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: @@ -66,7 +66,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: e2e - checkly - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: @@ -93,7 +93,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: e2e - create-checkly - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: