Skip to content

Commit 15618b6

Browse files
HweinstockHweinstock
authored
fix(telemetry): re-throw exceptions in withCommandRunTelemetry (#1437)
* refactor(telemetry): avoid rewrapping exceptions in results * fix(import): use result type to wrap error on invalid config * fix: avoid throws within blocks that return results * fix(logs): catch loadDeployedProjectConfig throws and update telemetry tests for re-throw * refactor(import): narrow addToProjectSpec return type to ImportResourceResult
1 parent c9d78ea commit 15618b6

16 files changed

Lines changed: 304 additions & 201 deletions

‎src/cli/commands/dev/command.tsx‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
findConfigRoot,
77
getWorkingDirectory,
88
} from '../../../lib';
9+
import { failureResult } from '../../../lib/result.js';
910
import { getErrorMessage } from '../../errors';
1011
import { detectContainerRuntime } from '../../external-requirements';
1112
import { isPreviewEnabled } from '../../feature-flags';
@@ -211,15 +212,19 @@ export const registerDev = (program: Command) => {
211212
},
212213
async recorder => {
213214
if (!positionalPrompt) {
214-
throw new ValidationError('A command is required with --exec. Usage: agentcore dev --exec "whoami"');
215+
return failureResult(
216+
new ValidationError('A command is required with --exec. Usage: agentcore dev --exec "whoami"')
217+
);
215218
}
216219
const workingDir = getWorkingDirectory();
217220
const project = await loadProjectConfig(workingDir);
218221
const agentName = opts.runtime ?? project?.runtimes[0]?.name ?? 'unknown';
219222
const targetAgent = project?.runtimes.find(a => a.name === agentName);
220223
if (targetAgent?.build !== 'Container') {
221-
throw new ValidationError(
222-
'--exec is only supported for Container build agents. For CodeZip agents, use your terminal to run commands directly.'
224+
return failureResult(
225+
new ValidationError(
226+
'--exec is only supported for Container build agents. For CodeZip agents, use your terminal to run commands directly.'
227+
)
223228
);
224229
}
225230
recorder.set({
@@ -230,8 +235,7 @@ export const registerDev = (program: Command) => {
230235
return { success: true as const };
231236
}
232237
);
233-
// TODO: Remove cast once withCommandRunTelemetry's return type is narrowed
234-
if (!execResult.success) throw (execResult as unknown as { error: Error }).error;
238+
if (!execResult.success) throw execResult.error;
235239
return;
236240
}
237241

‎src/cli/commands/import/__tests__/import-evaluator.test.ts‎

Lines changed: 45 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { toEvaluatorSpec } from '../import-evaluator';
1414
import { buildImportTemplate, findLogicalIdByProperty, findLogicalIdsByType } from '../template-utils';
1515
import type { CfnTemplate } from '../template-utils';
1616
import type { ResourceToImport } from '../types';
17-
import { describe, expect, it } from 'vitest';
17+
import { assert, describe, expect, it } from 'vitest';
1818

1919
// ============================================================================
2020
// toEvaluatorSpec Conversion Tests
@@ -46,19 +46,20 @@ describe('toEvaluatorSpec', () => {
4646

4747
const result = toEvaluatorSpec(detail, 'my_evaluator');
4848

49-
expect(result.name).toBe('my_evaluator');
50-
expect(result.level).toBe('SESSION');
51-
expect(result.description).toBe('Test evaluator');
52-
expect(result.config.llmAsAJudge).toBeDefined();
53-
expect(result.config.llmAsAJudge!.model).toBe('anthropic.claude-3-5-sonnet-20241022-v2:0');
54-
expect(result.config.llmAsAJudge!.instructions).toBe('Evaluate the response quality');
55-
expect(result.config.llmAsAJudge!.ratingScale.numerical).toHaveLength(2);
56-
expect(result.config.llmAsAJudge!.ratingScale.numerical![0]).toEqual({
49+
assert(result.success);
50+
expect(result.evaluator.name).toBe('my_evaluator');
51+
expect(result.evaluator.level).toBe('SESSION');
52+
expect(result.evaluator.description).toBe('Test evaluator');
53+
expect(result.evaluator.config.llmAsAJudge).toBeDefined();
54+
expect(result.evaluator.config.llmAsAJudge!.model).toBe('anthropic.claude-3-5-sonnet-20241022-v2:0');
55+
expect(result.evaluator.config.llmAsAJudge!.instructions).toBe('Evaluate the response quality');
56+
expect(result.evaluator.config.llmAsAJudge!.ratingScale.numerical).toHaveLength(2);
57+
expect(result.evaluator.config.llmAsAJudge!.ratingScale.numerical![0]).toEqual({
5758
value: 1,
5859
label: 'Poor',
5960
definition: 'Low quality response',
6061
});
61-
expect(result.tags).toEqual({ env: 'test' });
62+
expect(result.evaluator.tags).toEqual({ env: 'test' });
6263
});
6364

6465
it('maps LLM-as-a-Judge evaluator with categorical rating scale', () => {
@@ -84,16 +85,17 @@ describe('toEvaluatorSpec', () => {
8485

8586
const result = toEvaluatorSpec(detail, 'categorical_eval');
8687

87-
expect(result.level).toBe('TRACE');
88-
expect(result.config.llmAsAJudge).toBeDefined();
89-
expect(result.config.llmAsAJudge!.ratingScale.categorical).toHaveLength(2);
90-
expect(result.config.llmAsAJudge!.ratingScale.categorical![0]).toEqual({
88+
assert(result.success);
89+
expect(result.evaluator.level).toBe('TRACE');
90+
expect(result.evaluator.config.llmAsAJudge).toBeDefined();
91+
expect(result.evaluator.config.llmAsAJudge!.ratingScale.categorical).toHaveLength(2);
92+
expect(result.evaluator.config.llmAsAJudge!.ratingScale.categorical![0]).toEqual({
9193
label: 'Pass',
9294
definition: 'Response meets criteria',
9395
});
9496
// No description or tags
95-
expect(result.description).toBeUndefined();
96-
expect(result.tags).toBeUndefined();
97+
expect(result.evaluator.description).toBeUndefined();
98+
expect(result.evaluator.tags).toBeUndefined();
9799
});
98100

99101
it('maps code-based evaluator as external with Lambda ARN', () => {
@@ -112,14 +114,15 @@ describe('toEvaluatorSpec', () => {
112114

113115
const result = toEvaluatorSpec(detail, 'code_eval');
114116

115-
expect(result.name).toBe('code_eval');
116-
expect(result.level).toBe('TOOL_CALL');
117-
expect(result.config.codeBased).toBeDefined();
118-
expect(result.config.codeBased!.external).toBeDefined();
119-
expect(result.config.codeBased!.external!.lambdaArn).toBe(
117+
assert(result.success);
118+
expect(result.evaluator.name).toBe('code_eval');
119+
expect(result.evaluator.level).toBe('TOOL_CALL');
120+
expect(result.evaluator.config.codeBased).toBeDefined();
121+
expect(result.evaluator.config.codeBased!.external).toBeDefined();
122+
expect(result.evaluator.config.codeBased!.external!.lambdaArn).toBe(
120123
'arn:aws:lambda:us-west-2:123456789012:function:my-eval-function'
121124
);
122-
expect(result.config.llmAsAJudge).toBeUndefined();
125+
expect(result.evaluator.config.llmAsAJudge).toBeUndefined();
123126
});
124127

125128
it('uses provided local name instead of evaluator name from AWS', () => {
@@ -140,10 +143,11 @@ describe('toEvaluatorSpec', () => {
140143

141144
const result = toEvaluatorSpec(detail, 'custom_local_name');
142145

143-
expect(result.name).toBe('custom_local_name');
146+
assert(result.success);
147+
expect(result.evaluator.name).toBe('custom_local_name');
144148
});
145149

146-
it('throws when evaluator has no recognizable config', () => {
150+
it('returns failure when evaluator has no recognizable config', () => {
147151
const detail: GetEvaluatorResult = {
148152
evaluatorId: 'eval-no-config',
149153
evaluatorArn: 'arn:aws:bedrock-agentcore:us-west-2:123456789012:evaluator/eval-no-config',
@@ -152,10 +156,12 @@ describe('toEvaluatorSpec', () => {
152156
status: 'ACTIVE',
153157
};
154158

155-
expect(() => toEvaluatorSpec(detail, 'broken_eval')).toThrow('Evaluator "broken_eval" has no recognizable config');
159+
const result = toEvaluatorSpec(detail, 'broken_eval');
160+
assert(!result.success);
161+
expect(result.error.message).toContain('Evaluator "broken_eval" has no recognizable config');
156162
});
157163

158-
it('throws when evaluatorConfig is empty object', () => {
164+
it('returns failure when evaluatorConfig is empty object', () => {
159165
const detail: GetEvaluatorResult = {
160166
evaluatorId: 'eval-empty',
161167
evaluatorArn: 'arn:aws:bedrock-agentcore:us-west-2:123456789012:evaluator/eval-empty',
@@ -165,7 +171,9 @@ describe('toEvaluatorSpec', () => {
165171
evaluatorConfig: {},
166172
};
167173

168-
expect(() => toEvaluatorSpec(detail, 'empty_config_eval')).toThrow('has no recognizable config');
174+
const result = toEvaluatorSpec(detail, 'empty_config_eval');
175+
assert(!result.success);
176+
expect(result.error.message).toContain('has no recognizable config');
169177
});
170178

171179
it('omits description when not present', () => {
@@ -186,7 +194,8 @@ describe('toEvaluatorSpec', () => {
186194

187195
const result = toEvaluatorSpec(detail, 'no_desc_eval');
188196

189-
expect(result.description).toBeUndefined();
197+
assert(result.success);
198+
expect(result.evaluator.description).toBeUndefined();
190199
});
191200

192201
it('omits tags when empty', () => {
@@ -208,7 +217,8 @@ describe('toEvaluatorSpec', () => {
208217

209218
const result = toEvaluatorSpec(detail, 'empty_tags_eval');
210219

211-
expect(result.tags).toBeUndefined();
220+
assert(result.success);
221+
expect(result.evaluator.tags).toBeUndefined();
212222
});
213223

214224
it('forwards kmsKeyArn when present', () => {
@@ -230,7 +240,10 @@ describe('toEvaluatorSpec', () => {
230240

231241
const result = toEvaluatorSpec(detail, 'kms_eval');
232242

233-
expect(result.kmsKeyArn).toBe('arn:aws:kms:us-west-2:123456789012:key/12345678-1234-1234-1234-123456789012');
243+
assert(result.success);
244+
expect(result.evaluator.kmsKeyArn).toBe(
245+
'arn:aws:kms:us-west-2:123456789012:key/12345678-1234-1234-1234-123456789012'
246+
);
234247
});
235248

236249
it('omits kmsKeyArn when not present', () => {
@@ -251,7 +264,8 @@ describe('toEvaluatorSpec', () => {
251264

252265
const result = toEvaluatorSpec(detail, 'no_kms_eval');
253266

254-
expect(result.kmsKeyArn).toBeUndefined();
267+
assert(result.success);
268+
expect(result.evaluator.kmsKeyArn).toBeUndefined();
255269
});
256270
});
257271

‎src/cli/commands/import/__tests__/import-gateway-targets.test.ts‎

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*/
1111
import type { GatewayTargetDetail } from '../../../aws/agentcore-control';
1212
import { toGatewayTargetSpec } from '../import-gateway';
13-
import { describe, expect, it, vi } from 'vitest';
13+
import { assert, describe, expect, it, vi } from 'vitest';
1414

1515
/** Helper to build a minimal GatewayTargetDetail with only the fields under test. */
1616
function baseDetail(overrides: Partial<GatewayTargetDetail> = {}): GatewayTargetDetail {
@@ -48,12 +48,12 @@ describe('toGatewayTargetSpec — apiGateway', () => {
4848
const onProgress = vi.fn();
4949
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
5050

51-
expect(result).toBeDefined();
52-
expect(result!.name).toBe('test_target');
53-
expect(result!.targetType).toBe('apiGateway');
51+
assert(result.success);
52+
expect(result.target!.name).toBe('test_target');
53+
expect(result.target!.targetType).toBe('apiGateway');
5454

5555
// eslint-disable-next-line @typescript-eslint/no-explicit-any
56-
const apigw = (result as any).apiGateway;
56+
const apigw = (result.target as any).apiGateway;
5757
expect(apigw.restApiId).toBe('abc123');
5858
expect(apigw.stage).toBe('prod');
5959
expect(apigw.apiGatewayToolConfiguration.toolFilters).toEqual([
@@ -84,8 +84,9 @@ describe('toGatewayTargetSpec — apiGateway', () => {
8484
const onProgress = vi.fn();
8585
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
8686

87+
assert(result.success);
8788
// eslint-disable-next-line @typescript-eslint/no-explicit-any
88-
const apigw = (result as any).apiGateway;
89+
const apigw = (result.target as any).apiGateway;
8990
expect(apigw.apiGatewayToolConfiguration.toolOverrides).toEqual([
9091
{ name: 'listPets', path: '/pets', method: 'GET', description: 'List all pets' },
9192
{ name: 'createPet', path: '/pets', method: 'POST' },
@@ -110,8 +111,9 @@ describe('toGatewayTargetSpec — apiGateway', () => {
110111
const onProgress = vi.fn();
111112
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
112113

114+
assert(result.success);
113115
// eslint-disable-next-line @typescript-eslint/no-explicit-any
114-
const apigw = (result as any).apiGateway;
116+
const apigw = (result.target as any).apiGateway;
115117
expect(apigw.apiGatewayToolConfiguration.toolOverrides).toBeUndefined();
116118
});
117119

@@ -144,8 +146,8 @@ describe('toGatewayTargetSpec — apiGateway', () => {
144146
const onProgress = vi.fn();
145147
const result = toGatewayTargetSpec(detail, credentials, onProgress);
146148

147-
expect(result).toBeDefined();
148-
expect(result!.outboundAuth).toEqual({
149+
assert(result.success);
150+
expect(result.target!.outboundAuth).toEqual({
149151
type: 'OAUTH',
150152
credentialName: 'my_oauth_cred',
151153
scopes: ['read', 'write'],
@@ -172,12 +174,12 @@ describe('toGatewayTargetSpec — openApiSchema', () => {
172174
const onProgress = vi.fn();
173175
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
174176

175-
expect(result).toBeDefined();
176-
expect(result!.name).toBe('test_target');
177-
expect(result!.targetType).toBe('openApiSchema');
177+
assert(result.success);
178+
expect(result.target!.name).toBe('test_target');
179+
expect(result.target!.targetType).toBe('openApiSchema');
178180

179181
// eslint-disable-next-line @typescript-eslint/no-explicit-any
180-
const schemaSource = (result as any).schemaSource;
182+
const schemaSource = (result.target as any).schemaSource;
181183
expect(schemaSource.s3.uri).toBe('s3://my-bucket/schema.yaml');
182184
expect(schemaSource.s3.bucketOwnerAccountId).toBe('123456789012');
183185
});
@@ -194,7 +196,8 @@ describe('toGatewayTargetSpec — openApiSchema', () => {
194196
const onProgress = vi.fn();
195197
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
196198

197-
expect(result).toBeUndefined();
199+
assert(result.success);
200+
expect(result.target).toBeUndefined();
198201
expect(onProgress).toHaveBeenCalledWith(expect.stringContaining('(openApiSchema) has no S3 URI, skipping'));
199202
});
200203
});
@@ -218,12 +221,12 @@ describe('toGatewayTargetSpec — smithyModel', () => {
218221
const onProgress = vi.fn();
219222
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
220223

221-
expect(result).toBeDefined();
222-
expect(result!.name).toBe('test_target');
223-
expect(result!.targetType).toBe('smithyModel');
224+
assert(result.success);
225+
expect(result.target!.name).toBe('test_target');
226+
expect(result.target!.targetType).toBe('smithyModel');
224227

225228
// eslint-disable-next-line @typescript-eslint/no-explicit-any
226-
const schemaSource = (result as any).schemaSource;
229+
const schemaSource = (result.target as any).schemaSource;
227230
expect(schemaSource.s3.uri).toBe('s3://models-bucket/model.json');
228231
expect(schemaSource.s3.bucketOwnerAccountId).toBeUndefined();
229232
});
@@ -240,7 +243,8 @@ describe('toGatewayTargetSpec — smithyModel', () => {
240243
const onProgress = vi.fn();
241244
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
242245

243-
expect(result).toBeUndefined();
246+
assert(result.success);
247+
expect(result.target).toBeUndefined();
244248
expect(onProgress).toHaveBeenCalledWith(expect.stringContaining('(smithyModel) has no S3 URI, skipping'));
245249
});
246250
});
@@ -265,12 +269,12 @@ describe('toGatewayTargetSpec — lambda', () => {
265269
const onProgress = vi.fn();
266270
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
267271

268-
expect(result).toBeDefined();
269-
expect(result!.name).toBe('test_target');
270-
expect(result!.targetType).toBe('lambdaFunctionArn');
272+
assert(result.success);
273+
expect(result.target!.name).toBe('test_target');
274+
expect(result.target!.targetType).toBe('lambdaFunctionArn');
271275

272276
// eslint-disable-next-line @typescript-eslint/no-explicit-any
273-
const lambdaConfig = (result as any).lambdaFunctionArn;
277+
const lambdaConfig = (result.target as any).lambdaFunctionArn;
274278
expect(lambdaConfig.lambdaArn).toBe('arn:aws:lambda:us-west-2:123456789012:function:my-func');
275279
expect(lambdaConfig.toolSchemaFile).toBe('s3://schemas/tools.json');
276280
});
@@ -290,7 +294,8 @@ describe('toGatewayTargetSpec — lambda', () => {
290294
const onProgress = vi.fn();
291295
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
292296

293-
expect(result).toBeUndefined();
297+
assert(result.success);
298+
expect(result.target).toBeUndefined();
294299
expect(onProgress).toHaveBeenCalledWith(expect.stringContaining('(lambda) has no ARN, skipping'));
295300
});
296301

@@ -309,7 +314,8 @@ describe('toGatewayTargetSpec — lambda', () => {
309314
const onProgress = vi.fn();
310315
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
311316

312-
expect(result).toBeUndefined();
317+
assert(result.success);
318+
expect(result.target).toBeUndefined();
313319
expect(onProgress).toHaveBeenCalledWith(
314320
expect.stringContaining('has inline tool schema, which cannot be imported')
315321
);
@@ -349,7 +355,8 @@ describe('toGatewayTargetSpec — unrecognized target type', () => {
349355
const onProgress = vi.fn();
350356
const result = toGatewayTargetSpec(detail, new Map(), onProgress);
351357

352-
expect(result).toBeUndefined();
358+
assert(result.success);
359+
expect(result.target).toBeUndefined();
353360
expect(onProgress).toHaveBeenCalledWith(expect.stringContaining('unrecognized target type'));
354361
});
355362
});

0 commit comments

Comments
 (0)