feat(cli): add early diagnostics to Construct [RED-641] (#1355)

sorccu · claude · web-flow · commit 5a58188d0512 · 2026-06-23T10:32:26.000+09:00
* 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBbrKMRZw7DjdqjSgnQX41 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBbrKMRZw7DjdqjSgnQX41 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diff --git 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
@@ -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
@@ -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__/early-diagnostics.spec.ts 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/__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
@@ -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
@@ -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
@@ -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.
@@ -67,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
@@ -128,6 +142,8 @@ export abstract class Construct implements Validate, Bundle {
    */
   // eslint-disable-next-line require-await
   async validate (diagnostics: Diagnostics): Promise<void> {
+    diagnostics.extend(this.earlyDiagnostics)
+
     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
@@ -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
diff --git a/packages/cli/src/constructs/validator-error.ts b/packages/cli/src/constructs/validator-error.ts
