fix: emit ValidationError instead of UnknownError in create telemetry#1342
Conversation
578088f to
ee1869a
Compare
Package TarballHow to installgh release download pr-1342-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.2.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
The fix correctly identifies both root causes (plain Error vs ValidationError, and minification mangling class names), and keepNames: true is the right global fix that benefits every BaseError subclass.
One concern about completeness: the same throw new Error(validation.error) pattern exists in five other CLI command handlers, all inside runCliCommand blocks where the error feeds directly into classifyError. After this PR they will all still emit error_name: "UnknownError" for validation failures. See the inline comment for details.
|
Claude Security Review: no high-confidence findings. (run) |
Description
When the
createcommand fails validation (e.g. missing--framework,--model-provider,--memory), telemetry emitserror_name: "UnknownError"instead oferror_name: "ValidationError".Two root causes:
Plain
Errorthrown instead ofValidationError—command.tsxthrowsnew Error(validation.error). TheclassifyErrorfunction only recognizesBaseErrorsubclasses viainstanceof, so a plainErrorgets{ category: "UnknownError", source: "unknown" }.minify: truewithoutkeepNamesmangles class names —BaseErrorsetsthis.name = this.constructor.name. With minification, constructor names are mangled (e.g."Gr"), soErrorName.safeParse(err.name)fails for all error subclasses. Per esbuild docs:Fix
src/cli/commands/create/command.tsx— thrownew ValidationError(...)instead ofnew Error(...)esbuild.config.mjs— addkeepNames: trueto preserve class names through minificationinteg-tests/create-edge-cases.test.ts— asserterror_name: "ValidationError"anderror_source: "user"Related Issue
Closes #
Type of Change
Testing
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.