Skip to content

Commit ff5b20c

Browse files
HweinstockHweinstock
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 4f464d7 commit ff5b20c

134 files changed

Lines changed: 1352 additions & 1036 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/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: 23 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/result';
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,12 @@ 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+
};
4847
}
49-
return { success: false, error: `Failed to enable Application Signals: ${message}` };
48+
return { success: false, error: new Error(`Failed to enable Application Signals: ${message}`) };
5049
}
5150

5251
// Step 2: Create CloudWatch Logs resource policy for X-Ray (if needed)
@@ -80,9 +79,12 @@ export async function enableTransactionSearch(
8079
} catch (err: unknown) {
8180
const message = getErrorMessage(err);
8281
if (isAccessDeniedError(err)) {
83-
return { success: false, error: `Insufficient permissions to configure CloudWatch Logs policy: ${message}` };
82+
return {
83+
success: false,
84+
error: new AccessDeniedError(`Insufficient permissions to configure CloudWatch Logs policy: ${message}`),
85+
};
8486
}
85-
return { success: false, error: `Failed to configure CloudWatch Logs policy: ${message}` };
87+
return { success: false, error: new Error(`Failed to configure CloudWatch Logs policy: ${message}`) };
8688
}
8789

8890
const xrayClient = new XRayClient({ region, credentials });
@@ -96,9 +98,12 @@ export async function enableTransactionSearch(
9698
} catch (err: unknown) {
9799
const message = getErrorMessage(err);
98100
if (isAccessDeniedError(err)) {
99-
return { success: false, error: `Insufficient permissions to configure trace destination: ${message}` };
101+
return {
102+
success: false,
103+
error: new AccessDeniedError(`Insufficient permissions to configure trace destination: ${message}`),
104+
};
100105
}
101-
return { success: false, error: `Failed to configure trace destination: ${message}` };
106+
return { success: false, error: new Error(`Failed to configure trace destination: ${message}`) };
102107
}
103108

104109
// Step 4: Set indexing to 100% on the built-in Default rule (always exists, idempotent)
@@ -112,9 +117,12 @@ export async function enableTransactionSearch(
112117
} catch (err: unknown) {
113118
const message = getErrorMessage(err);
114119
if (isAccessDeniedError(err)) {
115-
return { success: false, error: `Insufficient permissions to configure indexing rules: ${message}` };
120+
return {
121+
success: false,
122+
error: new AccessDeniedError(`Insufficient permissions to configure indexing rules: ${message}`),
123+
};
116124
}
117-
return { success: false, error: `Failed to configure indexing rules: ${message}` };
125+
return { success: false, error: new Error(`Failed to configure indexing rules: ${message}`) };
118126
}
119127

120128
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: 12 additions & 9 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,7 @@ 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 { success: false, error: new DependencyCheckError(depCheck.errors), warnings: depWarnings };
6161
}
6262
}
6363

@@ -93,7 +93,11 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
9393
const gitResult = await initGitRepo(projectRoot);
9494
if (gitResult.status === 'error') {
9595
onProgress?.('Initialize git repository', 'error');
96-
return { success: false, error: gitResult.message, warnings: depWarnings };
96+
return {
97+
success: false,
98+
error: new GitInitError(gitResult.message ?? 'Git initialization failed'),
99+
warnings: depWarnings,
100+
};
97101
}
98102
onProgress?.('Initialize git repository', 'done');
99103
}
@@ -104,7 +108,7 @@ export async function createProject(options: CreateProjectOptions): Promise<Crea
104108
warnings: depWarnings.length > 0 ? depWarnings : undefined,
105109
};
106110
} catch (err) {
107-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
111+
return { success: false, error: toError(err), warnings: depWarnings };
108112
}
109113
}
110114

@@ -174,7 +178,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
174178

175179
// Fail on errors
176180
if (!depCheck.passed) {
177-
return { success: false, error: depCheck.errors.join('\n'), warnings: depWarnings };
181+
return { success: false, error: new DependencyCheckError(depCheck.errors), warnings: depWarnings };
178182
}
179183

180184
// First create the base project (skip dependency check since we already did it)
@@ -187,7 +191,6 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
187191
onProgress,
188192
});
189193
if (!projectResult.success) {
190-
// Merge warnings from both checks
191194
const allWarnings = [...depWarnings, ...(projectResult.warnings ?? [])];
192195
return { ...projectResult, warnings: allWarnings.length > 0 ? allWarnings : undefined };
193196
}
@@ -207,7 +210,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
207210
});
208211
if (!importResult.success) {
209212
onProgress?.('Import agent from Bedrock', 'error');
210-
return { success: false, error: importResult.error, warnings: depWarnings };
213+
return { ...importResult, warnings: depWarnings.length > 0 ? depWarnings : undefined };
211214
}
212215
onProgress?.('Import agent from Bedrock', 'done');
213216
return {
@@ -217,7 +220,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
217220
warnings: depWarnings.length > 0 ? depWarnings : undefined,
218221
};
219222
} catch (err) {
220-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
223+
return { success: false, error: toError(err), warnings: depWarnings };
221224
}
222225
}
223226

@@ -310,7 +313,7 @@ export async function createProjectWithAgent(options: CreateWithAgentOptions): P
310313
warnings: depWarnings.length > 0 ? depWarnings : undefined,
311314
};
312315
} catch (err) {
313-
return { success: false, error: getErrorMessage(err), warnings: depWarnings };
316+
return { success: false, error: toError(err), warnings: depWarnings };
314317
}
315318
}
316319

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { getWorkingDirectory } from '../../../lib';
1+
import { getWorkingDirectory, resultToJson } from '../../../lib';
22
import type {
33
BuildType,
44
ModelProvider,
@@ -93,8 +93,8 @@ async function handleCreateCLI(options: CreateOptions): Promise<void> {
9393
if (options.dryRun) {
9494
const result = getDryRunInfo({ name: name!, projectName, cwd, language: options.language });
9595
if (options.json) {
96-
console.log(JSON.stringify(result));
97-
} else {
96+
console.log(resultToJson(result));
97+
} else if (result.success) {
9898
console.log('Dry run - would create:');
9999
for (const path of result.wouldCreate ?? []) {
100100
console.log(` ${path}`);
@@ -158,7 +158,7 @@ async function handleCreateCLI(options: CreateOptions): Promise<void> {
158158
});
159159

160160
if (options.json) {
161-
console.log(JSON.stringify(result));
161+
console.log(resultToJson(result));
162162
} else if (result.success) {
163163
printCreateSummary(projectName!, result.agentName, options.language, options.framework);
164164
if (options.skipInstall) {
@@ -167,7 +167,7 @@ async function handleCreateCLI(options: CreateOptions): Promise<void> {
167167
);
168168
}
169169
} else {
170-
console.error(result.error);
170+
console.error(result.error.message);
171171
}
172172

173173
process.exit(result.success ? 0 : 1);

‎src/cli/commands/create/types.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,13 @@ export interface CreateOptions extends VpcOptions {
2828
json?: boolean;
2929
}
3030

31-
export interface CreateResult {
32-
success: boolean;
33-
projectPath?: string;
34-
agentName?: string;
35-
error?: string;
36-
dryRun?: boolean;
37-
wouldCreate?: string[];
38-
warnings?: string[];
39-
}
31+
export type CreateResult =
32+
| {
33+
success: true;
34+
projectPath?: string;
35+
agentName?: string;
36+
dryRun?: boolean;
37+
wouldCreate?: string[];
38+
warnings?: string[];
39+
}
40+
| { success: false; error: Error; warnings?: string[] };

0 commit comments

Comments
 (0)