Skip to content

Commit ad02465

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 ad02465

141 files changed

Lines changed: 1477 additions & 979 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: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1+
import { AgentAlreadyExistsError } from '../../lib';
12
import {
2-
AgentAlreadyExistsError,
33
getErrorMessage,
44
isChangesetInProgressError,
55
isExpiredTokenError,
66
isNoCredentialsError,
77
isStackInProgressError,
8+
toError,
89
} from '../errors.js';
910
import { describe, expect, it } from 'vitest';
1011

@@ -204,4 +205,35 @@ describe('errors', () => {
204205
expect(isChangesetInProgressError({})).toBe(false);
205206
});
206207
});
208+
209+
describe('toError', () => {
210+
it('returns Error instance as-is', () => {
211+
const err = new Error('original');
212+
expect(toError(err)).toBe(err);
213+
});
214+
215+
it('wraps string in Error', () => {
216+
const result = toError('string error');
217+
expect(result).toBeInstanceOf(Error);
218+
expect(result.message).toBe('string error');
219+
});
220+
221+
it('handles null', () => {
222+
const result = toError(null);
223+
expect(result).toBeInstanceOf(Error);
224+
expect(result.message).toBe('null');
225+
});
226+
227+
it('handles undefined', () => {
228+
const result = toError(undefined);
229+
expect(result).toBeInstanceOf(Error);
230+
expect(result.message).toBe('undefined');
231+
});
232+
233+
it('handles non-Error objects', () => {
234+
const result = toError({ code: 42 });
235+
expect(result).toBeInstanceOf(Error);
236+
expect(result.message).toBe('[object Object]');
237+
});
238+
});
207239
});

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 & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { AccessDeniedError } from '../../lib';
2+
import type { Result } from '../../lib/result';
13
import { getErrorMessage, isAccessDeniedError } from '../errors';
24
import { getCredentialProvider } from './account';
35
import { arnPrefix } from './partition';
@@ -14,11 +16,6 @@ import {
1416
XRayClient,
1517
} from '@aws-sdk/client-xray';
1618

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

2421
/**
@@ -34,7 +31,7 @@ export async function enableTransactionSearch(
3431
region: string,
3532
accountId: string,
3633
indexPercentage = 100
37-
): Promise<TransactionSearchEnableResult> {
34+
): Promise<Result> {
3835
const credentials = getCredentialProvider();
3936

4037
// Step 1: Enable Application Signals (creates service-linked role, idempotent)
@@ -44,9 +41,14 @@ export async function enableTransactionSearch(
4441
} catch (err: unknown) {
4542
const message = getErrorMessage(err);
4643
if (isAccessDeniedError(err)) {
47-
return { success: false, error: `Insufficient permissions to enable Application Signals: ${message}` };
44+
return {
45+
success: false,
46+
error: new AccessDeniedError(`Insufficient permissions to enable Application Signals: ${message}`, {
47+
cause: err,
48+
}),
49+
};
4850
}
49-
return { success: false, error: `Failed to enable Application Signals: ${message}` };
51+
return { success: false, error: new Error(`Failed to enable Application Signals: ${message}`, { cause: err }) };
5052
}
5153

5254
// Step 2: Create CloudWatch Logs resource policy for X-Ray (if needed)
@@ -80,9 +82,17 @@ export async function enableTransactionSearch(
8082
} catch (err: unknown) {
8183
const message = getErrorMessage(err);
8284
if (isAccessDeniedError(err)) {
83-
return { success: false, error: `Insufficient permissions to configure CloudWatch Logs policy: ${message}` };
85+
return {
86+
success: false,
87+
error: new AccessDeniedError(`Insufficient permissions to configure CloudWatch Logs policy: ${message}`, {
88+
cause: err,
89+
}),
90+
};
8491
}
85-
return { success: false, error: `Failed to configure CloudWatch Logs policy: ${message}` };
92+
return {
93+
success: false,
94+
error: new Error(`Failed to configure CloudWatch Logs policy: ${message}`, { cause: err }),
95+
};
8696
}
8797

8898
const xrayClient = new XRayClient({ region, credentials });
@@ -96,9 +106,14 @@ export async function enableTransactionSearch(
96106
} catch (err: unknown) {
97107
const message = getErrorMessage(err);
98108
if (isAccessDeniedError(err)) {
99-
return { success: false, error: `Insufficient permissions to configure trace destination: ${message}` };
109+
return {
110+
success: false,
111+
error: new AccessDeniedError(`Insufficient permissions to configure trace destination: ${message}`, {
112+
cause: err,
113+
}),
114+
};
100115
}
101-
return { success: false, error: `Failed to configure trace destination: ${message}` };
116+
return { success: false, error: new Error(`Failed to configure trace destination: ${message}`, { cause: err }) };
102117
}
103118

104119
// Step 4: Set indexing to 100% on the built-in Default rule (always exists, idempotent)
@@ -112,9 +127,14 @@ export async function enableTransactionSearch(
112127
} catch (err: unknown) {
113128
const message = getErrorMessage(err);
114129
if (isAccessDeniedError(err)) {
115-
return { success: false, error: `Insufficient permissions to configure indexing rules: ${message}` };
130+
return {
131+
success: false,
132+
error: new AccessDeniedError(`Insufficient permissions to configure indexing rules: ${message}`, {
133+
cause: err,
134+
}),
135+
};
116136
}
117-
return { success: false, error: `Failed to configure indexing rules: ${message}` };
137+
return { success: false, error: new Error(`Failed to configure indexing rules: ${message}`, { cause: err }) };
118138
}
119139

120140
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: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,12 @@
1-
import { APP_DIR, CONFIG_DIR, ConfigIO, setEnvVar, setSessionProjectRoot } from '../../../lib';
1+
import {
2+
APP_DIR,
3+
CONFIG_DIR,
4+
ConfigIO,
5+
DependencyCheckError,
6+
GitInitError,
7+
setEnvVar,
8+
setSessionProjectRoot,
9+
} from '../../../lib';
210
import type {
311
BuildType,
412
DeployedState,
@@ -8,7 +16,7 @@ import type {
816
SDKFramework,
917
TargetLanguage,
1018
} from '../../../schema';
11-
import { getErrorMessage } from '../../errors';
19+
import { toError } from '../../errors';
1220
import { checkCreateDependencies } from '../../external-requirements';
1321
import { initGitRepo, setupPythonProject, writeEnvFile, writeGitignore } from '../../operations';
1422
import { createConfigBundleForAgent } from '../../operations/agent/config-bundle-defaults';
@@ -57,7 +65,11 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
5765

5866
// Fail on errors
5967
if (!depCheck.passed) {
60-
return { success: false, error: depCheck.errors.join('\n'), warnings: depWarnings };
68+
return {
69+
success: false,
70+
error: new DependencyCheckError(depCheck.errors),
71+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
72+
};
6173
}
6274
}
6375

@@ -93,7 +105,11 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
93105
const gitResult = await initGitRepo(projectRoot);
94106
if (gitResult.status === 'error') {
95107
onProgress?.('Initialize git repository', 'error');
96-
return { success: false, error: gitResult.message, warnings: depWarnings };
108+
return {
109+
success: false,
110+
error: new GitInitError(gitResult.message ?? 'Git initialization failed'),
111+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
112+
};
97113
}
98114
onProgress?.('Initialize git repository', 'done');
99115
}
@@ -104,7 +120,7 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
104120
warnings: depWarnings.length > 0 ? depWarnings : undefined,
105121
};
106122
} catch (err) {
107-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
123+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
108124
}
109125
}
110126

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

175191
// Fail on errors
176192
if (!depCheck.passed) {
177-
return { success: false, error: depCheck.errors.join('\n'), warnings: depWarnings };
193+
return {
194+
success: false,
195+
error: new DependencyCheckError(depCheck.errors),
196+
warnings: depWarnings.length > 0 ? depWarnings : undefined,
197+
};
178198
}
179199

180200
// First create the base project (skip dependency check since we already did it)
@@ -187,9 +207,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
187207
onProgress,
188208
});
189209
if (!projectResult.success) {
190-
// Merge warnings from both checks
191-
const allWarnings = [...depWarnings, ...(projectResult.warnings ?? [])];
192-
return { ...projectResult, warnings: allWarnings.length > 0 ? allWarnings : undefined };
210+
return { ...projectResult, warnings: depWarnings.length > 0 ? depWarnings : projectResult.warnings };
193211
}
194212

195213
// Import path: delegate to executeImportAgent after project scaffolding
@@ -207,7 +225,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
207225
});
208226
if (!importResult.success) {
209227
onProgress?.('Import agent from Bedrock', 'error');
210-
return { success: false, error: importResult.error, warnings: depWarnings };
228+
return importResult;
211229
}
212230
onProgress?.('Import agent from Bedrock', 'done');
213231
return {
@@ -217,7 +235,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
217235
warnings: depWarnings.length > 0 ? depWarnings : undefined,
218236
};
219237
} catch (err) {
220-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
238+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
221239
}
222240
}
223241

@@ -310,7 +328,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
310328
warnings: depWarnings.length > 0 ? depWarnings : undefined,
311329
};
312330
} catch (err) {
313-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
331+
return { success: false, error: toError(err), warnings: depWarnings.length > 0 ? depWarnings : undefined };
314332
}
315333
}
316334

0 commit comments

Comments
 (0)