Skip to content

Commit 0177acb

Browse files
committed
refactor: unify result types with discriminated Result<T, E> union
Introduce a shared Result<T, E> type (inspired by Rust's Result) that replaces ad-hoc { success: boolean; error?: string } patterns across the codebase. Key changes: - Add src/lib/types.ts with Result<T, E> discriminated union type - Add toError() helper in src/cli/errors.ts for catch blocks - Migrate all command, operation, and primitive result types to Result<T> - Error field is now Error (not string) on the failure branch - Data fields only exist on the success branch (proper narrowing) - Update all consumers to narrow before accessing branch-specific fields - Update test assertions to match new Error objects and add narrowing
1 parent bceddde commit 0177acb

139 files changed

Lines changed: 1383 additions & 861 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

src/cli/__tests__/errors.test.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import {
55
isExpiredTokenError,
66
isNoCredentialsError,
77
isStackInProgressError,
8+
serializeResult,
9+
toError,
810
} from '../errors.js';
911
import { describe, expect, it } from 'vitest';
1012

@@ -204,4 +206,52 @@ describe('errors', () => {
204206
expect(isChangesetInProgressError({})).toBe(false);
205207
});
206208
});
209+
210+
describe('serializeResult', () => {
211+
it('passes through success results unchanged', () => {
212+
const result = { success: true as const, name: 'test' };
213+
expect(serializeResult(result)).toEqual({ success: true, name: 'test' });
214+
});
215+
216+
it('converts error.message to string on failure', () => {
217+
const result = { success: false as const, error: new Error('something broke') };
218+
expect(serializeResult(result)).toEqual({ success: false, error: 'something broke' });
219+
});
220+
221+
it('preserves extra fields on failure branch', () => {
222+
const result = { success: false as const, error: new Error('fail'), logPath: '/tmp/log' };
223+
expect(serializeResult(result)).toEqual({ success: false, error: 'fail', logPath: '/tmp/log' });
224+
});
225+
});
226+
227+
describe('toError', () => {
228+
it('returns Error instance as-is', () => {
229+
const err = new Error('original');
230+
expect(toError(err)).toBe(err);
231+
});
232+
233+
it('wraps string in Error', () => {
234+
const result = toError('string error');
235+
expect(result).toBeInstanceOf(Error);
236+
expect(result.message).toBe('string error');
237+
});
238+
239+
it('handles null', () => {
240+
const result = toError(null);
241+
expect(result).toBeInstanceOf(Error);
242+
expect(result.message).toBe('null');
243+
});
244+
245+
it('handles undefined', () => {
246+
const result = toError(undefined);
247+
expect(result).toBeInstanceOf(Error);
248+
expect(result.message).toBe('undefined');
249+
});
250+
251+
it('handles non-Error objects', () => {
252+
const result = toError({ code: 42 });
253+
expect(result).toBeInstanceOf(Error);
254+
expect(result.message).toBe('[object Object]');
255+
});
256+
});
207257
});

src/cli/aws/__tests__/transaction-search.test.ts

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { enableTransactionSearch } from '../transaction-search.js';
2+
import assert from 'node:assert';
23
import { beforeEach, describe, expect, it, vi } from 'vitest';
34

45
const { mockAppSignalsSend, mockLogsSend, mockXRaySend } = vi.hoisted(() => ({
@@ -162,17 +163,17 @@ describe('enableTransactionSearch', () => {
162163

163164
const result = await enableTransactionSearch('us-east-1', '123456789012');
164165

165-
expect(result.success).toBe(false);
166-
expect(result.error).toContain('Insufficient permissions to enable Application Signals');
166+
assert(!result.success);
167+
expect(result.error.message).toContain('Insufficient permissions to enable Application Signals');
167168
});
168169

169170
it('returns error when Application Signals fails with generic error', async () => {
170171
mockAppSignalsSend.mockRejectedValue(new Error('Service unavailable'));
171172

172173
const result = await enableTransactionSearch('us-east-1', '123456789012');
173174

174-
expect(result.success).toBe(false);
175-
expect(result.error).toContain('Failed to enable Application Signals');
175+
assert(!result.success);
176+
expect(result.error.message).toContain('Failed to enable Application Signals');
176177
});
177178

178179
it('returns error when CloudWatch Logs policy fails with AccessDenied', async () => {
@@ -183,8 +184,8 @@ describe('enableTransactionSearch', () => {
183184

184185
const result = await enableTransactionSearch('us-east-1', '123456789012');
185186

186-
expect(result.success).toBe(false);
187-
expect(result.error).toContain('Insufficient permissions to configure CloudWatch Logs policy');
187+
assert(!result.success);
188+
expect(result.error.message).toContain('Insufficient permissions to configure CloudWatch Logs policy');
188189
});
189190

190191
it('returns error when trace destination fails', async () => {
@@ -194,8 +195,8 @@ describe('enableTransactionSearch', () => {
194195

195196
const result = await enableTransactionSearch('us-east-1', '123456789012');
196197

197-
expect(result.success).toBe(false);
198-
expect(result.error).toContain('Failed to configure trace destination');
198+
assert(!result.success);
199+
expect(result.error.message).toContain('Failed to configure trace destination');
199200
});
200201

201202
it('returns error when indexing rule update fails', async () => {
@@ -214,8 +215,8 @@ describe('enableTransactionSearch', () => {
214215

215216
const result = await enableTransactionSearch('us-east-1', '123456789012');
216217

217-
expect(result.success).toBe(false);
218-
expect(result.error).toContain('Failed to configure indexing rules');
218+
assert(!result.success);
219+
expect(result.error.message).toContain('Failed to configure indexing rules');
219220
});
220221

221222
it('does not proceed to later steps when an earlier step fails', async () => {

src/cli/aws/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ export {
1717
type GetAgentRuntimeStatusOptions,
1818
} from './agentcore-control';
1919
export { streamLogs, searchLogs, type LogEvent, type StreamLogsOptions, type SearchLogsOptions } from './cloudwatch';
20-
export { enableTransactionSearch, type TransactionSearchEnableResult } from './transaction-search';
20+
export { enableTransactionSearch } from './transaction-search';
2121
export {
2222
startPolicyGeneration,
2323
getPolicyGeneration,

src/cli/aws/transaction-search.ts

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { getErrorMessage, isAccessDeniedError } from '../errors';
1+
import type { Result } from '../../lib/types';
2+
import { AccessDeniedError, getErrorMessage, isAccessDeniedError } from '../errors';
23
import { getCredentialProvider } from './account';
34
import { arnPrefix } from './partition';
45
import { ApplicationSignalsClient, StartDiscoveryCommand } from '@aws-sdk/client-application-signals';
@@ -14,11 +15,6 @@ import {
1415
XRayClient,
1516
} from '@aws-sdk/client-xray';
1617

17-
export interface TransactionSearchEnableResult {
18-
success: boolean;
19-
error?: string;
20-
}
21-
2218
const RESOURCE_POLICY_NAME = 'TransactionSearchXRayAccess';
2319

2420
/**
@@ -34,7 +30,7 @@ export async function enableTransactionSearch(
3430
region: string,
3531
accountId: string,
3632
indexPercentage = 100
37-
): Promise<TransactionSearchEnableResult> {
33+
): Promise<Result> {
3834
const credentials = getCredentialProvider();
3935

4036
// Step 1: Enable Application Signals (creates service-linked role, idempotent)
@@ -44,9 +40,14 @@ export async function enableTransactionSearch(
4440
} catch (err: unknown) {
4541
const message = getErrorMessage(err);
4642
if (isAccessDeniedError(err)) {
47-
return { success: false, error: `Insufficient permissions to enable Application Signals: ${message}` };
43+
return {
44+
success: false,
45+
error: new AccessDeniedError(`Insufficient permissions to enable Application Signals: ${message}`, {
46+
cause: err,
47+
}),
48+
};
4849
}
49-
return { success: false, error: `Failed to enable Application Signals: ${message}` };
50+
return { success: false, error: new Error(`Failed to enable Application Signals: ${message}`, { cause: err }) };
5051
}
5152

5253
// Step 2: Create CloudWatch Logs resource policy for X-Ray (if needed)
@@ -80,9 +81,17 @@ export async function enableTransactionSearch(
8081
} catch (err: unknown) {
8182
const message = getErrorMessage(err);
8283
if (isAccessDeniedError(err)) {
83-
return { success: false, error: `Insufficient permissions to configure CloudWatch Logs policy: ${message}` };
84+
return {
85+
success: false,
86+
error: new AccessDeniedError(`Insufficient permissions to configure CloudWatch Logs policy: ${message}`, {
87+
cause: err,
88+
}),
89+
};
8490
}
85-
return { success: false, error: `Failed to configure CloudWatch Logs policy: ${message}` };
91+
return {
92+
success: false,
93+
error: new Error(`Failed to configure CloudWatch Logs policy: ${message}`, { cause: err }),
94+
};
8695
}
8796

8897
const xrayClient = new XRayClient({ region, credentials });
@@ -96,9 +105,14 @@ export async function enableTransactionSearch(
96105
} catch (err: unknown) {
97106
const message = getErrorMessage(err);
98107
if (isAccessDeniedError(err)) {
99-
return { success: false, error: `Insufficient permissions to configure trace destination: ${message}` };
108+
return {
109+
success: false,
110+
error: new AccessDeniedError(`Insufficient permissions to configure trace destination: ${message}`, {
111+
cause: err,
112+
}),
113+
};
100114
}
101-
return { success: false, error: `Failed to configure trace destination: ${message}` };
115+
return { success: false, error: new Error(`Failed to configure trace destination: ${message}`, { cause: err }) };
102116
}
103117

104118
// Step 4: Set indexing to 100% on the built-in Default rule (always exists, idempotent)
@@ -112,9 +126,14 @@ export async function enableTransactionSearch(
112126
} catch (err: unknown) {
113127
const message = getErrorMessage(err);
114128
if (isAccessDeniedError(err)) {
115-
return { success: false, error: `Insufficient permissions to configure indexing rules: ${message}` };
129+
return {
130+
success: false,
131+
error: new AccessDeniedError(`Insufficient permissions to configure indexing rules: ${message}`, {
132+
cause: err,
133+
}),
134+
};
116135
}
117-
return { success: false, error: `Failed to configure indexing rules: ${message}` };
136+
return { success: false, error: new Error(`Failed to configure indexing rules: ${message}`, { cause: err }) };
118137
}
119138

120139
return { success: true };

src/cli/commands/add/types.ts

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,6 @@ export interface AddAgentOptions extends VpcOptions {
4141
json?: boolean;
4242
}
4343

44-
export interface AddAgentResult {
45-
success: boolean;
46-
agentName?: string;
47-
agentPath?: string;
48-
error?: string;
49-
}
50-
5144
// Gateway types
5245
export interface AddGatewayOptions {
5346
name?: string;
@@ -68,12 +61,6 @@ export interface AddGatewayOptions {
6861
json?: boolean;
6962
}
7063

71-
export interface AddGatewayResult {
72-
success: boolean;
73-
gatewayName?: string;
74-
error?: string;
75-
}
76-
7764
// Gateway Target types
7865
export interface AddGatewayTargetOptions {
7966
name?: string;
@@ -100,13 +87,6 @@ export interface AddGatewayTargetOptions {
10087
json?: boolean;
10188
}
10289

103-
export interface AddGatewayTargetResult {
104-
success: boolean;
105-
toolName?: string;
106-
sourcePath?: string;
107-
error?: string;
108-
}
109-
11090
// Memory types (v2: no owner/user concept)
11191
export interface AddMemoryOptions {
11292
name?: string;
@@ -119,12 +99,6 @@ export interface AddMemoryOptions {
11999
json?: boolean;
120100
}
121101

122-
export interface AddMemoryResult {
123-
success: boolean;
124-
memoryName?: string;
125-
error?: string;
126-
}
127-
128102
// Credential types (v2: credential, no owner/user concept)
129103
export interface AddCredentialOptions {
130104
name?: string;
@@ -139,9 +113,3 @@ export interface AddCredentialOptions {
139113

140114
/** @deprecated Use AddCredentialOptions */
141115
export type AddIdentityOptions = AddCredentialOptions;
142-
143-
export interface AddCredentialResult {
144-
success: boolean;
145-
credentialName?: string;
146-
error?: string;
147-
}

src/cli/commands/create/action.ts

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import type {
88
SDKFramework,
99
TargetLanguage,
1010
} from '../../../schema';
11-
import { getErrorMessage } from '../../errors';
11+
import { DependencyCheckError, GitInitError, toError } from '../../errors';
1212
import { checkCreateDependencies } from '../../external-requirements';
1313
import { initGitRepo, setupPythonProject, writeEnvFile, writeGitignore } from '../../operations';
1414
import { createConfigBundleForAgent } from '../../operations/agent/config-bundle-defaults';
@@ -57,7 +57,11 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
5757

5858
// Fail on errors
5959
if (!depCheck.passed) {
60-
return { success: false, error: depCheck.errors.join('\n'), warnings: depWarnings };
60+
return {
61+
success: false,
62+
error: new DependencyCheckError(depCheck.errors),
63+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
64+
};
6165
}
6266
}
6367

@@ -93,7 +97,11 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
9397
const gitResult = await initGitRepo(projectRoot);
9498
if (gitResult.status === 'error') {
9599
onProgress?.('Initialize git repository', 'error');
96-
return { success: false, error: gitResult.message, warnings: depWarnings };
100+
return {
101+
success: false,
102+
error: new GitInitError(gitResult.message ?? 'Git initialization failed'),
103+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
104+
};
97105
}
98106
onProgress?.('Initialize git repository', 'done');
99107
}
@@ -104,7 +112,7 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
104112
warnings: depWarnings.length > 0 ? depWarnings : undefined,
105113
};
106114
} catch (err) {
107-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
115+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
108116
}
109117
}
110118

@@ -174,7 +182,11 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
174182

175183
// Fail on errors
176184
if (!depCheck.passed) {
177-
return { success: false, error: depCheck.errors.join('\n'), warnings: depWarnings };
185+
return {
186+
success: false,
187+
error: new DependencyCheckError(depCheck.errors),
188+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
189+
};
178190
}
179191

180192
// First create the base project (skip dependency check since we already did it)
@@ -187,9 +199,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
187199
onProgress,
188200
});
189201
if (!projectResult.success) {
190-
// Merge warnings from both checks
191-
const allWarnings = [...depWarnings, ...(projectResult.warnings ?? [])];
192-
return { ...projectResult, warnings: allWarnings.length > 0 ? allWarnings : undefined };
202+
return { ...projectResult, warnings: depWarnings.length > 0 ? depWarnings : projectResult.warnings };
193203
}
194204

195205
// Import path: delegate to executeImportAgent after project scaffolding
@@ -207,7 +217,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
207217
});
208218
if (!importResult.success) {
209219
onProgress?.('Import agent from Bedrock', 'error');
210-
return { success: false, error: importResult.error, warnings: depWarnings };
220+
return importResult;
211221
}
212222
onProgress?.('Import agent from Bedrock', 'done');
213223
return {
@@ -217,7 +227,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
217227
warnings: depWarnings.length > 0 ? depWarnings : undefined,
218228
};
219229
} catch (err) {
220-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
230+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
221231
}
222232
}
223233

@@ -310,7 +320,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
310320
warnings: depWarnings.length > 0 ? depWarnings : undefined,
311321
};
312322
} catch (err) {
313-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
323+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
314324
}
315325
}
316326

0 commit comments

Comments
 (0)