From 4e0f723d63c392d3aaa9f3d6c1a467d4c8eec499 Mon Sep 17 00:00:00 2001 From: miliberlin Date: Wed, 18 Feb 2026 11:24:16 +0100 Subject: [PATCH 1/3] fix: convert remaining hard errors to soft diagnostics for logicalId validation [red-71] Non-string logicalId and duplicate logicalId errors were still thrown as hard errors during construct construction, stopping on the first error. Convert both to soft diagnostics so all issues are reported together. Co-Authored-By: Claude Opus 4.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 | 10 +++- packages/cli/src/constructs/project.ts | 16 +++--- 7 files changed, 135 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts index 0456ad337..176625738 100644 --- a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts +++ b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts @@ -18,19 +18,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 04cf633b7..6be63d1c3 100644 --- a/packages/cli/src/constructs/__tests__/check-group.spec.ts +++ b/packages/cli/src/constructs/__tests__/check-group.spec.ts @@ -9,20 +9,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', () => { @@ -79,20 +87,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 08fb6ec3c..8080a91d8 100644 --- a/packages/cli/src/constructs/__tests__/private-location.spec.ts +++ b/packages/cli/src/constructs/__tests__/private-location.spec.ts @@ -4,21 +4,30 @@ import { PrivateLocation, Diagnostics } from '../index' import { Project, Session } from '../project' 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 e36f3bf68..af3cbf833 100644 --- a/packages/cli/src/constructs/__tests__/project.spec.ts +++ b/packages/cli/src/constructs/__tests__/project.spec.ts @@ -1,11 +1,11 @@ import { describe, it, expect, beforeEach } from 'vitest' -import { Session } from '../project' +import { Project, Session } from '../project' import { Construct } from '../construct' import { Diagnostics } from '../diagnostics' class TestConstruct extends Construct { - constructor (logicalId: string) { + constructor (logicalId: any) { super('test', logicalId) } @@ -74,4 +74,35 @@ describe('Construct logicalId validation', () => { }), ])) }) + + it('should produce a diagnostic for non-string logicalId instead of throwing', async () => { + const construct = new TestConstruct(123 as any) + 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 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({ + 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 f913195f8..704a095d7 100644 --- a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts +++ b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts @@ -4,20 +4,28 @@ import { StatusPageService, Diagnostics } from '../index' import { Project, Session } from '../project' 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 48b1be679..a16ebe7ef 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -56,6 +56,7 @@ export abstract class Construct implements Validate, Bundle { member: boolean /** Absolute path to the check file that created this construct */ checkFileAbsolutePath?: string + private _originalLogicalIdType: string /** * Creates a new construct instance. @@ -66,7 +67,8 @@ 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) { - this.logicalId = logicalId + this._originalLogicalIdType = typeof logicalId + this.logicalId = typeof logicalId === 'string' ? logicalId : String(logicalId) this.type = type this.physicalId = physicalId this.member = member ?? true @@ -127,6 +129,12 @@ export abstract class Construct implements Validate, Bundle { */ // eslint-disable-next-line require-await async validate (diagnostics: Diagnostics): Promise { + if (this._originalLogicalIdType !== 'string') { + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`Expected a string but received type "${this._originalLogicalIdType}".`), + )) + } if (!LOGICAL_ID_PATTERN.test(this.logicalId)) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'logicalId', diff --git a/packages/cli/src/constructs/project.ts b/packages/cli/src/constructs/project.ts index 8cf1f8e34..194693103 100644 --- a/packages/cli/src/constructs/project.ts +++ b/packages/cli/src/constructs/project.ts @@ -4,7 +4,6 @@ import * as api from '../rest/api' import { CheckConfigDefaults } from '../services/checkly-config-loader' import { Parser } from '../services/check-parser/parser' import { Construct } from './construct' -import { ValidationError } from './validator-error' import { Check, AlertChannelSubscription, AlertChannel, CheckGroup, MaintenanceWindow, Dashboard, @@ -62,6 +61,7 @@ export class Project extends Construct { repoUrl?: string logicalId: string testOnlyAllowed = false + duplicateResources: Array<{ type: string, logicalId: string }> = [] data: ProjectData = { 'check': {}, 'check-group': {}, @@ -98,6 +98,13 @@ export class Project extends Construct { async validate (diagnostics: Diagnostics): Promise { await super.validate(diagnostics) + for (const { type, logicalId } of this.duplicateResources) { + diagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`A ${type} with logicalId "${logicalId}" already exists.`), + )) + } + if (!this.name) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'name', @@ -134,7 +141,8 @@ export class Project extends Construct { return } - throw new Error(`Resource of type '${type}' with logical id '${logicalId}' already exists.`) + this.duplicateResources.push({ type, logicalId }) + return } this.data[type as keyof ProjectData][logicalId] = resource @@ -324,10 +332,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.__checklyType) { // Creating the construct is allowed - We're creating the project. } else if (Session.project) { From 58d7efd04c57ed5a6b5d57df066281f66cac4add Mon Sep 17 00:00:00 2001 From: miliberlin Date: Wed, 18 Feb 2026 13:10:28 +0100 Subject: [PATCH 2/3] fix: update e2e test to expect soft diagnostic message for duplicate logicalId Co-Authored-By: Claude Opus 4.6 --- packages/cli/e2e/__tests__/test.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 4a35fba04..6bc16410b 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -169,7 +169,7 @@ describe('test', { timeout: 45000 }, () => { } 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.') + .toContain('A check-group with logicalId "my-check-group" already exists.') expect(err.exitCode).toBe(1) } else { throw err From 732f802413f039e7cad10c4162d0f751fc7825ad Mon Sep 17 00:00:00 2001 From: miliberlin Date: Wed, 18 Feb 2026 14:02:38 +0100 Subject: [PATCH 3/3] fix: check stdout instead of stderr for soft diagnostic in e2e test Soft diagnostics are printed via log() to stdout, not stderr. Co-Authored-By: Claude Opus 4.6 --- packages/cli/e2e/__tests__/test.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 6bc16410b..5fc2f9eb2 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -168,7 +168,7 @@ describe('test', { timeout: 45000 }, () => { await runTest(fixt, []) } catch (err) { if (err instanceof ExecaError) { - expect((err.stderr as unknown as string).replace(/(\n {4})/gm, '')) + 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 {