fix: validate logicalId via diagnostics instead of throwing at construction time [red-71] by miliberlin · Pull Request #1205 · checkly/checkly-cli

miliberlin · 2026-02-04T08:03:11Z

Summary

Instead of throwing a hard error at parse time when a logicalId contains invalid characters, we now report it as a validation diagnostic. This means:

Parsing completes successfully even with invalid logicalIds
All invalid logicalIds are reported at once during validation
Users can fix everything in one go instead of fixing one, re-running, finding the next, etc.

No sanitization or interactive prompts — just clear error diagnostics.

Example

Parsing your project... ✅

Validating project resources... ❌
✖ Invalid property value

  The value provided for property "logicalId" is not valid.

  Reason: The logicalId "boilerplate project (invalid)" contains invalid
  characters. Only A-Z, a-z, 0-9, _, -, /, #, and . are allowed.

✖ [ApiCheck:books api check (v1)] Invalid property value

  The value provided for property "logicalId" is not valid.

  Reason: The logicalId "books api check (v1)" contains invalid characters.
  Only A-Z, a-z, 0-9, _, -, /, #, and . are allowed.

✖ Your project is not valid.

Affected Components

Changes

Move logicalId character validation from Session.validateCreateConstruct (constructor-time error) to Construct.validate() (diagnostic)
Remove sanitizeLogicalId and interactive confirmation prompt approach
Delete helpers/sanitized-logical-id.ts
Extract LOGICAL_ID_PATTERN to constants.ts for reuse
Convert CheckTypes from default export to named export
Add tests for logicalId validation via diagnostics

ferrandiaz · 2026-02-04T09:32:33Z

packages/cli/src/constructs/project.ts

it would be weird for the user if suddenly the logicalId changes, how about giving a warn on what is going on and what are we going to do? also, they will still have the invalid logicalId on their config right?

I updated the PR to prompt confirmation and not just overwrite the logicalIds, but yes, the source code will remain unchanged.

Instead of silently sanitizing logicalIds with invalid characters, prompt the user for confirmation before proceeding. This makes it clear what changes will be applied when syncing with Checkly. - Track sanitized logicalIds in Session - Add confirmation prompt in deploy, test, and pw-test commands - Explain that source files won't be modified - Show valid character set to help users fix their config - Fix Project constructor to use sanitized logicalId from Construct Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Extract the duplicated confirmation prompt code into a reusable helper function to reduce code duplication across deploy, test, and pw-test commands. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

sorccu · 2026-02-05T10:14:46Z

I will write some comments later, please don't merge yet.

miliberlin · 2026-02-05T10:19:42Z

@sorccu No worries, I'm not merging this before proper review! I'm a bit skeptical about these changes too.
We might want to keep the error as before and just improve the messaging. I don't know how much of a pain this validation error is but in the end the user will still have to update their source code to conform with our validation rules or they will see the message every time. (cc @ferrandiaz regarding your earlier comment)

…trip-invalid-characters-from-project-name-in-checkly # Conflicts: # packages/cli/src/constructs/project.ts

miliberlin · 2026-02-17T07:17:05Z

@sorccu what are your thoughts on this? I'm also okay with keeping the previous flow. It's a bit wonky to create a diversion between local code and what we run. We could also just try to make the existing error message a bit more elaborate and keep the error.

sorccu · 2026-02-17T07:24:44Z

I would want to remove the current validation from the constructor and use the validate（） method instead. Of course, there are some drawbacks if the construct can store a potentially unclean value, but as long as the validation is error level I think it should be good enough and far better than the current behavior we have.

I would rather not interactively ask the user.

sorccu · 2026-02-17T07:26:46Z

If you use the validate method, you can use the hidden validate command to try it out.

npx checkly validate

…uction time Move logicalId character validation from Session.validateCreateConstruct (which threw a hard error at parse time) to Construct.validate() (which reports diagnostics). This lets users see all invalid logicalIds at once instead of fixing them one at a time. Also extracts LOGICAL_ID_PATTERN to constants.ts and converts CheckTypes to a named export. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

miliberlin · 2026-02-17T10:49:20Z

packages/cli/src/constructs/project.ts

 export class Project extends Construct {
  name: string
  repoUrl?: string
  logicalId: string


Nit: Project.logicalId re-declares and re-assigns the property already defined on the base Construct class. We could remove it to avoid the shadowing, but I kept it for now to minimize changes. Should we clean this up or keep for clarity?

miliberlin · 2026-02-17T10:51:02Z

@sorccu I updated the approach, can you check?

…-from-project-name-in-checkly

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sorccu

This still leaves the following two cases uncovered:

If the logicalId is not a string, a hard ValidationError will be thrown.
The two resources of the same type have the same logicalId, a hard ValidationError will be thrown.

We should restructure the code so that soft validation could be used. However, it would make the PR far more complex. The current change is already an improvement over current behavior even if it doesn't cover all cases.

packages/cli/src/constructs/__tests__/project.spec.ts

packages/cli/src/constructs/construct.ts

sorccu

Actually, I'd like to see my comments about the tests addressed before this can be merged.

Use expect.arrayContaining/objectContaining/stringContaining pattern in tests instead of checking exact diagnostic types. Remove property name prefix from error message per review nit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

miliberlin · 2026-02-17T17:08:44Z

This still leaves the following two cases uncovered:

If the logicalId is not a string, a hard ValidationError will be thrown.

The two resources of the same type have the same logicalId, a hard ValidationError will be thrown.

We should restructure the code so that soft validation could be used. However, it would make the PR far more complex. The current change is already an improvement over current behavior even if it doesn't cover all cases.

Good points. I'll create another ticket for that.

Implemented your feedback so far 👍

…-from-project-name-in-checkly

miliberlin added 2 commits February 4, 2026 09:01

feat: normalize logicalId [red-71]

67437c6

feat: push test [red-71]

2d1e992

miliberlin changed the title ~~feat: normalize logicalId [red-71]~~ feat: normalize logicalId instead of throwing error [red-71] Feb 4, 2026

miliberlin requested review from ferrandiaz and sorccu February 4, 2026 09:29

ferrandiaz reviewed Feb 4, 2026

View reviewed changes

miliberlin and others added 2 commits February 5, 2026 11:03

refactor: extract sanitized logicalId confirmation to helper [red-71]

37f35cd

Extract the duplicated confirmation prompt code into a reusable helper function to reduce code duplication across deploy, test, and pw-test commands. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/main' into michelle/red-71-cli-s…

f0bc9cd

…trip-invalid-characters-from-project-name-in-checkly # Conflicts: # packages/cli/src/constructs/project.ts

miliberlin requested a review from ferrandiaz February 17, 2026 07:17

miliberlin changed the title ~~feat: normalize logicalId instead of throwing error [red-71]~~ fix: validate logicalId via diagnostics instead of throwing at construction time [red-71] Feb 17, 2026

fix: restore logicalId property declaration on Project class

f4ae237

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

miliberlin commented Feb 17, 2026

View reviewed changes

miliberlin and others added 2 commits February 17, 2026 11:51

Merge branch 'main' into michelle/red-71-cli-strip-invalid-characters…

b0adb3b

…-from-project-name-in-checkly

fix: update deploy e2e test to use named CheckTypes import

2dffaa6

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sorccu approved these changes Feb 17, 2026

View reviewed changes