Skip to content

Commit a261366

Browse files
tejaskashtejaskash
committed
fix: address review feedback on lifecycle configuration
- Extract LIFECYCLE_TIMEOUT_MIN/MAX constants from schema, use everywhere instead of hardcoded 60/28800 values (review comment 2) - Rename validateLifecycleOptions → parseAndValidateLifecycleOptions to clarify intent; return parsed values instead of mutating input (comment 3) - Unify create/add option types to number | string for consistency (comment 4)
1 parent ad06702 commit a261366

File tree

13 files changed

+119
-83
lines changed

13 files changed

+119
-83
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ export interface AddAgentOptions extends VpcOptions {
1818
agentId?: string;
1919
agentAliasId?: string;
2020
region?: string;
21-
idleTimeout?: number;
22-
maxLifetime?: number;
21+
idleTimeout?: number | string;
22+
maxLifetime?: number | string;
2323
json?: boolean;
2424
}
2525

‎src/cli/commands/add/validate.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
getSupportedModelProviders,
1515
matchEnumValue,
1616
} from '../../../schema';
17-
import { validateLifecycleOptions } from '../shared/lifecycle-utils';
17+
import { parseAndValidateLifecycleOptions } from '../shared/lifecycle-utils';
1818
import { validateVpcOptions } from '../shared/vpc-utils';
1919
import type {
2020
AddAgentOptions,
@@ -137,9 +137,11 @@ export function validateAddAgentOptions(options: AddAgentOptions): ValidationRes
137137
error: `Invalid memory option: ${options.memory}. Use none, shortTerm, or longAndShortTerm`,
138138
};
139139
}
140-
// Validate lifecycle configuration for import path
141-
const lcResult = validateLifecycleOptions(options);
140+
// Parse and validate lifecycle configuration for import path
141+
const lcResult = parseAndValidateLifecycleOptions(options);
142142
if (!lcResult.valid) return lcResult;
143+
if (lcResult.idleTimeout !== undefined) options.idleTimeout = lcResult.idleTimeout;
144+
if (lcResult.maxLifetime !== undefined) options.maxLifetime = lcResult.maxLifetime;
143145

144146
// Force import defaults
145147
options.modelProvider = 'Bedrock' as typeof options.modelProvider;
@@ -171,9 +173,11 @@ export function validateAddAgentOptions(options: AddAgentOptions): ValidationRes
171173
return { valid: false, error: '--code-location is required for BYO path' };
172174
}
173175

174-
// Validate lifecycle configuration for MCP path
175-
const mcpLcResult = validateLifecycleOptions(options);
176+
// Parse and validate lifecycle configuration for MCP path
177+
const mcpLcResult = parseAndValidateLifecycleOptions(options);
176178
if (!mcpLcResult.valid) return mcpLcResult;
179+
if (mcpLcResult.idleTimeout !== undefined) options.idleTimeout = mcpLcResult.idleTimeout;
180+
if (mcpLcResult.maxLifetime !== undefined) options.maxLifetime = mcpLcResult.maxLifetime;
177181

178182
return { valid: true };
179183
}
@@ -243,9 +247,11 @@ export function validateAddAgentOptions(options: AddAgentOptions): ValidationRes
243247
}
244248
}
245249

246-
// Validate lifecycle configuration
247-
const lifecycleResult = validateLifecycleOptions(options);
250+
// Parse and validate lifecycle configuration
251+
const lifecycleResult = parseAndValidateLifecycleOptions(options);
248252
if (!lifecycleResult.valid) return lifecycleResult;
253+
if (lifecycleResult.idleTimeout !== undefined) options.idleTimeout = lifecycleResult.idleTimeout;
254+
if (lifecycleResult.maxLifetime !== undefined) options.maxLifetime = lifecycleResult.maxLifetime;
249255

250256
// Validate VPC options
251257
const vpcResult = validateVpcOptions(options);

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import type {
77
SDKFramework,
88
TargetLanguage,
99
} from '../../../schema';
10+
import { LIFECYCLE_TIMEOUT_MAX, LIFECYCLE_TIMEOUT_MIN } from '../../../schema';
1011
import { getErrorMessage } from '../../errors';
1112
import { COMMAND_DESCRIPTIONS } from '../../tui/copy';
1213
import { CreateScreen } from '../../tui/screens/create';
@@ -178,8 +179,14 @@ export const registerCreate = (program: Command) => {
178179
.option('--network-mode <mode>', 'Network mode (PUBLIC, VPC) [non-interactive]')
179180
.option('--subnets <ids>', 'Comma-separated subnet IDs (required for VPC mode) [non-interactive]')
180181
.option('--security-groups <ids>', 'Comma-separated security group IDs (required for VPC mode) [non-interactive]')
181-
.option('--idle-timeout <seconds>', 'Idle session timeout in seconds (60-28800) [non-interactive]')
182-
.option('--max-lifetime <seconds>', 'Max instance lifetime in seconds (60-28800) [non-interactive]')
182+
.option(
183+
'--idle-timeout <seconds>',
184+
`Idle session timeout in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}) [non-interactive]`
185+
)
186+
.option(
187+
'--max-lifetime <seconds>',
188+
`Max instance lifetime in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}) [non-interactive]`
189+
)
183190
.option('--output-dir <dir>', 'Output directory (default: current directory) [non-interactive]')
184191
.option('--skip-git', 'Skip git repository initialization [non-interactive]')
185192
.option('--skip-python-setup', 'Skip Python virtual environment setup [non-interactive]')

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ export interface CreateOptions extends VpcOptions {
1515
agentId?: string;
1616
agentAliasId?: string;
1717
region?: string;
18-
idleTimeout?: string;
19-
maxLifetime?: string;
18+
idleTimeout?: number | string;
19+
maxLifetime?: number | string;
2020
outputDir?: string;
2121
skipGit?: boolean;
2222
skipPythonSetup?: boolean;

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
matchEnumValue,
1111
} from '../../../schema';
1212
import type { ProtocolMode } from '../../../schema';
13-
import { validateLifecycleOptions } from '../shared/lifecycle-utils';
13+
import { parseAndValidateLifecycleOptions } from '../shared/lifecycle-utils';
1414
import { validateVpcOptions } from '../shared/vpc-utils';
1515
import type { CreateOptions } from './types';
1616
import { existsSync } from 'fs';
@@ -201,9 +201,11 @@ export function validateCreateOptions(options: CreateOptions, cwd?: string): Val
201201
return { valid: false, error: vpcResult.error };
202202
}
203203

204-
// Validate lifecycle configuration
205-
const lifecycleResult = validateLifecycleOptions(options);
204+
// Parse and validate lifecycle configuration
205+
const lifecycleResult = parseAndValidateLifecycleOptions(options);
206206
if (!lifecycleResult.valid) return lifecycleResult;
207+
if (lifecycleResult.idleTimeout !== undefined) options.idleTimeout = lifecycleResult.idleTimeout;
208+
if (lifecycleResult.maxLifetime !== undefined) options.maxLifetime = lifecycleResult.maxLifetime;
207209

208210
return { valid: true };
209211
}

‎src/cli/commands/shared/__tests__/lifecycle-utils.test.ts‎

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,91 +1,92 @@
1-
import { validateLifecycleOptions } from '../lifecycle-utils';
1+
import { parseAndValidateLifecycleOptions } from '../lifecycle-utils';
22
import { describe, expect, it } from 'vitest';
33

4-
describe('validateLifecycleOptions', () => {
4+
describe('parseAndValidateLifecycleOptions', () => {
55
it('returns valid when no options are set', () => {
6-
expect(validateLifecycleOptions({})).toEqual({ valid: true });
6+
expect(parseAndValidateLifecycleOptions({})).toEqual({ valid: true });
77
});
88

9-
it('accepts valid idleTimeout', () => {
10-
const opts = { idleTimeout: 900 };
11-
expect(validateLifecycleOptions(opts)).toEqual({ valid: true });
12-
expect(opts.idleTimeout).toBe(900);
9+
it('accepts valid idleTimeout and returns parsed value', () => {
10+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 900 });
11+
expect(result).toEqual({ valid: true, idleTimeout: 900 });
1312
});
1413

15-
it('accepts valid maxLifetime', () => {
16-
const opts = { maxLifetime: 3600 };
17-
expect(validateLifecycleOptions(opts)).toEqual({ valid: true });
18-
expect(opts.maxLifetime).toBe(3600);
14+
it('accepts valid maxLifetime and returns parsed value', () => {
15+
const result = parseAndValidateLifecycleOptions({ maxLifetime: 3600 });
16+
expect(result).toEqual({ valid: true, maxLifetime: 3600 });
1917
});
2018

2119
it('accepts both when idle <= max', () => {
22-
expect(validateLifecycleOptions({ idleTimeout: 600, maxLifetime: 3600 })).toEqual({ valid: true });
20+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 600, maxLifetime: 3600 });
21+
expect(result).toEqual({ valid: true, idleTimeout: 600, maxLifetime: 3600 });
2322
});
2423

2524
it('accepts boundary values (60 and 28800)', () => {
26-
expect(validateLifecycleOptions({ idleTimeout: 60, maxLifetime: 28800 })).toEqual({ valid: true });
25+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 60, maxLifetime: 28800 });
26+
expect(result).toEqual({ valid: true, idleTimeout: 60, maxLifetime: 28800 });
2727
});
2828

2929
it('accepts equal values', () => {
30-
expect(validateLifecycleOptions({ idleTimeout: 3600, maxLifetime: 3600 })).toEqual({ valid: true });
30+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 3600, maxLifetime: 3600 });
31+
expect(result).toEqual({ valid: true, idleTimeout: 3600, maxLifetime: 3600 });
3132
});
3233

3334
it('rejects idleTimeout below 60', () => {
34-
const result = validateLifecycleOptions({ idleTimeout: 59 });
35+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 59 });
3536
expect(result.valid).toBe(false);
3637
expect(result.error).toContain('--idle-timeout');
3738
});
3839

3940
it('rejects idleTimeout above 28800', () => {
40-
const result = validateLifecycleOptions({ idleTimeout: 28801 });
41+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 28801 });
4142
expect(result.valid).toBe(false);
4243
expect(result.error).toContain('--idle-timeout');
4344
});
4445

4546
it('rejects maxLifetime below 60', () => {
46-
const result = validateLifecycleOptions({ maxLifetime: 59 });
47+
const result = parseAndValidateLifecycleOptions({ maxLifetime: 59 });
4748
expect(result.valid).toBe(false);
4849
expect(result.error).toContain('--max-lifetime');
4950
});
5051

5152
it('rejects maxLifetime above 28800', () => {
52-
const result = validateLifecycleOptions({ maxLifetime: 28801 });
53+
const result = parseAndValidateLifecycleOptions({ maxLifetime: 28801 });
5354
expect(result.valid).toBe(false);
5455
expect(result.error).toContain('--max-lifetime');
5556
});
5657

5758
it('rejects idle > max', () => {
58-
const result = validateLifecycleOptions({ idleTimeout: 5000, maxLifetime: 3000 });
59+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 5000, maxLifetime: 3000 });
5960
expect(result.valid).toBe(false);
6061
expect(result.error).toContain('--idle-timeout must be <= --max-lifetime');
6162
});
6263

6364
it('rejects non-integer idleTimeout', () => {
64-
const result = validateLifecycleOptions({ idleTimeout: 120.5 });
65+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 120.5 });
6566
expect(result.valid).toBe(false);
6667
expect(result.error).toContain('--idle-timeout');
6768
});
6869

6970
it('rejects NaN string idleTimeout', () => {
70-
const result = validateLifecycleOptions({ idleTimeout: 'abc' as unknown as number });
71+
const result = parseAndValidateLifecycleOptions({ idleTimeout: 'abc' });
7172
expect(result.valid).toBe(false);
7273
expect(result.error).toContain('--idle-timeout');
7374
});
7475

7576
it('rejects NaN string maxLifetime', () => {
76-
const result = validateLifecycleOptions({ maxLifetime: 'abc' as unknown as number });
77+
const result = parseAndValidateLifecycleOptions({ maxLifetime: 'abc' });
7778
expect(result.valid).toBe(false);
7879
expect(result.error).toContain('--max-lifetime');
7980
});
8081

81-
it('normalizes string values to numbers', () => {
82-
const opts: { idleTimeout?: number | string; maxLifetime?: number | string } = {
83-
idleTimeout: '300',
84-
maxLifetime: '7200',
85-
};
86-
const result = validateLifecycleOptions(opts);
82+
it('parses string values to numbers without mutating input', () => {
83+
const opts = { idleTimeout: '300', maxLifetime: '7200' };
84+
const result = parseAndValidateLifecycleOptions(opts);
8785
expect(result.valid).toBe(true);
88-
expect(opts.idleTimeout).toBe(300);
89-
expect(opts.maxLifetime).toBe(7200);
86+
expect(result.idleTimeout).toBe(300);
87+
expect(result.maxLifetime).toBe(7200);
88+
// Original input is NOT mutated
89+
expect(opts.idleTimeout).toBe('300');
90+
expect(opts.maxLifetime).toBe('7200');
9091
});
9192
});

‎src/cli/commands/shared/lifecycle-utils.ts‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { LIFECYCLE_TIMEOUT_MAX, LIFECYCLE_TIMEOUT_MIN } from '../../../schema';
2+
13
export interface LifecycleOptions {
24
idleTimeout?: number | string;
35
maxLifetime?: number | string;
@@ -6,36 +8,43 @@ export interface LifecycleOptions {
68
export interface LifecycleValidationResult {
79
valid: boolean;
810
error?: string;
11+
idleTimeout?: number;
12+
maxLifetime?: number;
913
}
1014

11-
export const LIFECYCLE_MIN = 60;
12-
export const LIFECYCLE_MAX = 28800;
15+
/**
16+
* Parse and validate lifecycle CLI options.
17+
* Coerces string values to numbers and validates range constraints.
18+
* Returns the parsed numeric values in the result — does NOT mutate the input.
19+
*/
20+
export function parseAndValidateLifecycleOptions(options: LifecycleOptions): LifecycleValidationResult {
21+
let idleTimeout: number | undefined;
22+
let maxLifetime: number | undefined;
1323

14-
export function validateLifecycleOptions(options: LifecycleOptions): LifecycleValidationResult {
1524
if (options.idleTimeout !== undefined) {
1625
const val = Number(options.idleTimeout);
17-
if (isNaN(val) || !Number.isInteger(val) || val < LIFECYCLE_MIN || val > LIFECYCLE_MAX) {
26+
if (isNaN(val) || !Number.isInteger(val) || val < LIFECYCLE_TIMEOUT_MIN || val > LIFECYCLE_TIMEOUT_MAX) {
1827
return {
1928
valid: false,
20-
error: `--idle-timeout must be an integer between ${LIFECYCLE_MIN} and ${LIFECYCLE_MAX} seconds`,
29+
error: `--idle-timeout must be an integer between ${LIFECYCLE_TIMEOUT_MIN} and ${LIFECYCLE_TIMEOUT_MAX} seconds`,
2130
};
2231
}
23-
options.idleTimeout = val;
32+
idleTimeout = val;
2433
}
2534
if (options.maxLifetime !== undefined) {
2635
const val = Number(options.maxLifetime);
27-
if (isNaN(val) || !Number.isInteger(val) || val < LIFECYCLE_MIN || val > LIFECYCLE_MAX) {
36+
if (isNaN(val) || !Number.isInteger(val) || val < LIFECYCLE_TIMEOUT_MIN || val > LIFECYCLE_TIMEOUT_MAX) {
2837
return {
2938
valid: false,
30-
error: `--max-lifetime must be an integer between ${LIFECYCLE_MIN} and ${LIFECYCLE_MAX} seconds`,
39+
error: `--max-lifetime must be an integer between ${LIFECYCLE_TIMEOUT_MIN} and ${LIFECYCLE_TIMEOUT_MAX} seconds`,
3140
};
3241
}
33-
options.maxLifetime = val;
42+
maxLifetime = val;
3443
}
35-
if (options.idleTimeout !== undefined && options.maxLifetime !== undefined) {
36-
if (Number(options.idleTimeout) > Number(options.maxLifetime)) {
44+
if (idleTimeout !== undefined && maxLifetime !== undefined) {
45+
if (idleTimeout > maxLifetime) {
3746
return { valid: false, error: '--idle-timeout must be <= --max-lifetime' };
3847
}
3948
}
40-
return { valid: true };
49+
return { valid: true, idleTimeout, maxLifetime };
4150
}

‎src/cli/primitives/AgentPrimitive.tsx‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type {
1010
SDKFramework,
1111
TargetLanguage,
1212
} from '../../schema';
13-
import { AgentEnvSpecSchema, CREDENTIAL_PROVIDERS } from '../../schema';
13+
import { AgentEnvSpecSchema, CREDENTIAL_PROVIDERS, LIFECYCLE_TIMEOUT_MAX, LIFECYCLE_TIMEOUT_MIN } from '../../schema';
1414
import type { AddAgentOptions as CLIAddAgentOptions } from '../commands/add/types';
1515
import { validateAddAgentOptions } from '../commands/add/validate';
1616
import type { VpcOptions } from '../commands/shared/vpc-utils';
@@ -204,8 +204,14 @@ export class AgentPrimitive extends BasePrimitive<AddAgentOptions, RemovableReso
204204
.option('--network-mode <mode>', 'Network mode (PUBLIC, VPC) [non-interactive]')
205205
.option('--subnets <ids>', 'Comma-separated subnet IDs (required for VPC mode) [non-interactive]')
206206
.option('--security-groups <ids>', 'Comma-separated security group IDs (required for VPC mode) [non-interactive]')
207-
.option('--idle-timeout <seconds>', 'Idle session timeout in seconds (60-28800) [non-interactive]')
208-
.option('--max-lifetime <seconds>', 'Max instance lifetime in seconds (60-28800) [non-interactive]')
207+
.option(
208+
'--idle-timeout <seconds>',
209+
`Idle session timeout in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}) [non-interactive]`
210+
)
211+
.option(
212+
'--max-lifetime <seconds>',
213+
`Max instance lifetime in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}) [non-interactive]`
214+
)
209215
.option('--json', 'Output as JSON [non-interactive]')
210216
.action(async options => {
211217
if (!findConfigRoot()) {

‎src/cli/tui/screens/agent/AddAgentScreen.tsx‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { APP_DIR, ConfigIO } from '../../../../lib';
22
import type { ModelProvider, NetworkMode, SDKFramework } from '../../../../schema';
3-
import { AgentNameSchema, DEFAULT_MODEL_IDS } from '../../../../schema';
3+
import { AgentNameSchema, DEFAULT_MODEL_IDS, LIFECYCLE_TIMEOUT_MAX, LIFECYCLE_TIMEOUT_MIN } from '../../../../schema';
44
import { listBedrockAgentAliases, listBedrockAgents } from '../../../aws/bedrock-import';
55
import type { BedrockAgentSummary, BedrockAliasSummary } from '../../../aws/bedrock-import-types';
66
import { parseAndNormalizeHeaders, validateHeaderAllowlist } from '../../../commands/shared/header-utils';
@@ -886,13 +886,13 @@ export function AddAgentScreen({ existingAgentNames, onComplete, onExit }: AddAg
886886

887887
{byoStep === 'idleTimeout' && (
888888
<TextInput
889-
prompt="Idle session timeout in seconds (60-28800, or press Enter to skip)"
889+
prompt={`Idle session timeout in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}, or press Enter to skip)`}
890890
initialValue=""
891891
customValidation={value => {
892892
if (!value) return true;
893893
const n = Number(value);
894-
if (isNaN(n) || !Number.isInteger(n) || n < 60 || n > 28800)
895-
return 'Must be an integer between 60 and 28800';
894+
if (isNaN(n) || !Number.isInteger(n) || n < LIFECYCLE_TIMEOUT_MIN || n > LIFECYCLE_TIMEOUT_MAX)
895+
return `Must be an integer between ${LIFECYCLE_TIMEOUT_MIN} and ${LIFECYCLE_TIMEOUT_MAX}`;
896896
return true;
897897
}}
898898
onSubmit={value => {
@@ -905,13 +905,13 @@ export function AddAgentScreen({ existingAgentNames, onComplete, onExit }: AddAg
905905

906906
{byoStep === 'maxLifetime' && (
907907
<TextInput
908-
prompt="Max instance lifetime in seconds (60-28800, or press Enter to skip)"
908+
prompt={`Max instance lifetime in seconds (${LIFECYCLE_TIMEOUT_MIN}-${LIFECYCLE_TIMEOUT_MAX}, or press Enter to skip)`}
909909
initialValue=""
910910
customValidation={value => {
911911
if (!value) return true;
912912
const n = Number(value);
913-
if (isNaN(n) || !Number.isInteger(n) || n < 60 || n > 28800)
914-
return 'Must be an integer between 60 and 28800';
913+
if (isNaN(n) || !Number.isInteger(n) || n < LIFECYCLE_TIMEOUT_MIN || n > LIFECYCLE_TIMEOUT_MAX)
914+
return `Must be an integer between ${LIFECYCLE_TIMEOUT_MIN} and ${LIFECYCLE_TIMEOUT_MAX}`;
915915
if (byoConfig.idleTimeout) {
916916
const idle = Number(byoConfig.idleTimeout);
917917
if (!isNaN(idle) && n < idle) return 'Must be >= idle timeout';

‎src/cli/tui/screens/agent/types.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,9 @@ export interface AddAgentConfig {
7979
securityGroups?: string[];
8080
/** Allowed request headers for the runtime */
8181
requestHeaderAllowlist?: string[];
82-
/** Idle session timeout in seconds (60-28800) */
82+
/** Idle session timeout in seconds (LIFECYCLE_TIMEOUT_MIN-LIFECYCLE_TIMEOUT_MAX) */
8383
idleRuntimeSessionTimeout?: number;
84-
/** Max instance lifetime in seconds (60-28800) */
84+
/** Max instance lifetime in seconds (LIFECYCLE_TIMEOUT_MIN-LIFECYCLE_TIMEOUT_MAX) */
8585
maxLifetime?: number;
8686
/** Python version (only for Python agents) */
8787
pythonVersion: PythonRuntime;

0 commit comments

Comments
 (0)