Skip to content

Commit 92c7469

Browse files
tejaskashtejaskash
committed
fix: require gateway when creating a gateway target
A gateway target must always be attached to a gateway. Previously it was possible to create unassigned targets via "Skip for now" in the TUI or by omitting --gateway in non-interactive mode. - Validation now requires --gateway and verifies the gateway exists - TUI removes the "Skip for now" option from gateway selection - createExternalGatewayTarget rejects missing gateway at operations layer - Updated tests to cover all new validation paths
1 parent 5d9d710 commit 92c7469

6 files changed

Lines changed: 100 additions & 81 deletions

File tree

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

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,21 @@ import {
1212
validateAddIdentityOptions,
1313
validateAddMemoryOptions,
1414
} from '../validate.js';
15-
import { afterEach, describe, expect, it, vi } from 'vitest';
15+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
1616

1717
const mockReadProjectSpec = vi.fn();
18+
const mockGetExistingGateways = vi.fn();
1819

1920
vi.mock('../../../../lib/index.js', () => ({
2021
ConfigIO: class {
2122
readProjectSpec = mockReadProjectSpec;
2223
},
2324
}));
2425

26+
vi.mock('../../../operations/mcp/create-mcp.js', () => ({
27+
getExistingGateways: (...args: unknown[]) => mockGetExistingGateways(...args),
28+
}));
29+
2530
// Helper: valid base options for each type
2631
const validAgentOptionsByo: AddAgentOptions = {
2732
name: 'TestAgent',
@@ -238,19 +243,48 @@ describe('validate', () => {
238243
});
239244

240245
describe('validateAddGatewayTargetOptions', () => {
246+
beforeEach(() => {
247+
// By default, mock that the gateway from validGatewayTargetOptions exists
248+
mockGetExistingGateways.mockResolvedValue(['my-gateway']);
249+
});
250+
241251
// AC15: Required fields validated
242-
it('returns error for missing required fields', async () => {
243-
const requiredFields: { field: keyof AddGatewayTargetOptions; error: string }[] = [
244-
{ field: 'name', error: '--name is required' },
245-
{ field: 'language', error: '--language is required' },
246-
];
252+
it('returns error for missing name', async () => {
253+
const opts = { ...validGatewayTargetOptions, name: undefined };
254+
const result = await validateAddGatewayTargetOptions(opts);
255+
expect(result.valid).toBe(false);
256+
expect(result.error).toBe('--name is required');
257+
});
247258

248-
for (const { field, error } of requiredFields) {
249-
const opts = { ...validGatewayTargetOptions, [field]: undefined };
250-
const result = await validateAddGatewayTargetOptions(opts);
251-
expect(result.valid, `Should fail for missing ${String(field)}`).toBe(false);
252-
expect(result.error).toBe(error);
253-
}
259+
it('returns error for missing language (non-existing-endpoint)', async () => {
260+
const opts = { ...validGatewayTargetOptions, language: undefined };
261+
const result = await validateAddGatewayTargetOptions(opts);
262+
expect(result.valid).toBe(false);
263+
expect(result.error).toBe('--language is required');
264+
});
265+
266+
// Gateway is required
267+
it('returns error when --gateway is missing', async () => {
268+
const opts = { ...validGatewayTargetOptions, gateway: undefined };
269+
const result = await validateAddGatewayTargetOptions(opts);
270+
expect(result.valid).toBe(false);
271+
expect(result.error).toContain('--gateway is required');
272+
});
273+
274+
it('returns error when no gateways exist', async () => {
275+
mockGetExistingGateways.mockResolvedValue([]);
276+
const result = await validateAddGatewayTargetOptions(validGatewayTargetOptions);
277+
expect(result.valid).toBe(false);
278+
expect(result.error).toContain('No gateways found');
279+
expect(result.error).toContain('agentcore add gateway');
280+
});
281+
282+
it('returns error when specified gateway does not exist', async () => {
283+
mockGetExistingGateways.mockResolvedValue(['other-gateway']);
284+
const result = await validateAddGatewayTargetOptions(validGatewayTargetOptions);
285+
expect(result.valid).toBe(false);
286+
expect(result.error).toContain('Gateway "my-gateway" not found');
287+
expect(result.error).toContain('other-gateway');
254288
});
255289

256290
// AC16: Invalid values rejected
@@ -274,6 +308,7 @@ describe('validate', () => {
274308
name: 'test-tool',
275309
source: 'existing-endpoint',
276310
endpoint: 'https://example.com/mcp',
311+
gateway: 'my-gateway',
277312
};
278313
const result = await validateAddGatewayTargetOptions(options);
279314
expect(result.valid).toBe(true);
@@ -285,6 +320,7 @@ describe('validate', () => {
285320
name: 'test-tool',
286321
source: 'existing-endpoint',
287322
endpoint: 'http://localhost:3000/mcp',
323+
gateway: 'my-gateway',
288324
};
289325
const result = await validateAddGatewayTargetOptions(options);
290326
expect(result.valid).toBe(true);
@@ -294,6 +330,7 @@ describe('validate', () => {
294330
const options: AddGatewayTargetOptions = {
295331
name: 'test-tool',
296332
source: 'existing-endpoint',
333+
gateway: 'my-gateway',
297334
};
298335
const result = await validateAddGatewayTargetOptions(options);
299336
expect(result.valid).toBe(false);
@@ -305,6 +342,7 @@ describe('validate', () => {
305342
name: 'test-tool',
306343
source: 'existing-endpoint',
307344
endpoint: 'ftp://example.com/mcp',
345+
gateway: 'my-gateway',
308346
};
309347
const result = await validateAddGatewayTargetOptions(options);
310348
expect(result.valid).toBe(false);
@@ -316,6 +354,7 @@ describe('validate', () => {
316354
name: 'test-tool',
317355
source: 'existing-endpoint',
318356
endpoint: 'not-a-url',
357+
gateway: 'my-gateway',
319358
};
320359
const result = await validateAddGatewayTargetOptions(options);
321360
expect(result.valid).toBe(false);
@@ -331,6 +370,7 @@ describe('validate', () => {
331370
const options: AddGatewayTargetOptions = {
332371
name: 'test-tool',
333372
language: 'Python',
373+
gateway: 'my-gateway',
334374
outboundAuthType: 'API_KEY',
335375
credentialName: 'missing-cred',
336376
};
@@ -347,6 +387,7 @@ describe('validate', () => {
347387
const options: AddGatewayTargetOptions = {
348388
name: 'test-tool',
349389
language: 'Python',
390+
gateway: 'my-gateway',
350391
outboundAuthType: 'API_KEY',
351392
credentialName: 'any-cred',
352393
};
@@ -363,6 +404,7 @@ describe('validate', () => {
363404
const options: AddGatewayTargetOptions = {
364405
name: 'test-tool',
365406
language: 'Python',
407+
gateway: 'my-gateway',
366408
outboundAuthType: 'API_KEY',
367409
credentialName: 'valid-cred',
368410
};
@@ -429,6 +471,7 @@ describe('validate', () => {
429471
source: 'existing-endpoint',
430472
endpoint: 'https://example.com/mcp',
431473
host: 'Lambda',
474+
gateway: 'my-gateway',
432475
};
433476
const result = await validateAddGatewayTargetOptions(options);
434477
expect(result.valid).toBe(false);

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
TargetLanguageSchema,
99
getSupportedModelProviders,
1010
} from '../../../schema';
11+
import { getExistingGateways } from '../../operations/mcp/create-mcp';
1112
import type {
1213
AddAgentOptions,
1314
AddGatewayOptions,
@@ -197,6 +198,29 @@ export async function validateAddGatewayTargetOptions(options: AddGatewayTargetO
197198
return { valid: false, error: 'Invalid source. Valid options: existing-endpoint, create-new' };
198199
}
199200

201+
// Gateway is required — a gateway target must be attached to a gateway
202+
if (!options.gateway) {
203+
return {
204+
valid: false,
205+
error: "--gateway is required. A gateway target must be attached to a gateway. Create a gateway first with 'agentcore add gateway'.",
206+
};
207+
}
208+
209+
// Validate the specified gateway exists
210+
const existingGateways = await getExistingGateways();
211+
if (existingGateways.length === 0) {
212+
return {
213+
valid: false,
214+
error: "No gateways found. Create a gateway first with 'agentcore add gateway' before adding a gateway target.",
215+
};
216+
}
217+
if (!existingGateways.includes(options.gateway)) {
218+
return {
219+
valid: false,
220+
error: `Gateway "${options.gateway}" not found. Available gateways: ${existingGateways.join(', ')}`,
221+
};
222+
}
223+
200224
if (options.source === 'existing-endpoint') {
201225
if (options.host) {
202226
return { valid: false, error: '--host is not applicable for existing endpoint targets' };
@@ -216,7 +240,6 @@ export async function validateAddGatewayTargetOptions(options: AddGatewayTargetO
216240

217241
// Populate defaults for fields skipped by external endpoint flow
218242
options.language ??= 'Other';
219-
options.gateway ??= undefined;
220243

221244
return { valid: true };
222245
}

‎src/cli/operations/mcp/__tests__/create-mcp.test.ts‎

Lines changed: 5 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { SKIP_FOR_NOW } from '../../../tui/screens/mcp/types.js';
21
import type { AddGatewayConfig, AddGatewayTargetConfig } from '../../../tui/screens/mcp/types.js';
32
import { createExternalGatewayTarget, createGatewayFromWizard, getUnassignedTargets } from '../create-mcp.js';
43
import { afterEach, describe, expect, it, vi } from 'vitest';
@@ -55,29 +54,14 @@ describe('createExternalGatewayTarget', () => {
5554
expect(gateway.targets[0]!.targetType).toBe('mcpServer');
5655
});
5756

58-
it('stores target in unassignedTargets when gateway is skip-for-now', async () => {
59-
const mockMcpSpec = { agentCoreGateways: [] };
60-
mockConfigExists.mockReturnValue(true);
61-
mockReadMcpSpec.mockResolvedValue(mockMcpSpec);
62-
63-
await createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW }));
64-
65-
expect(mockWriteMcpSpec).toHaveBeenCalled();
66-
const written = mockWriteMcpSpec.mock.calls[0]![0];
67-
expect(written.unassignedTargets).toHaveLength(1);
68-
expect(written.unassignedTargets[0]!.name).toBe('test-target');
69-
expect(written.unassignedTargets[0]!.endpoint).toBe('https://api.example.com');
70-
});
71-
72-
it('initializes unassignedTargets array if it does not exist in mcp spec', async () => {
73-
const mockMcpSpec = { agentCoreGateways: [] };
57+
it('throws when gateway is not provided', async () => {
58+
const mockMcpSpec = { agentCoreGateways: [{ name: 'test-gateway', targets: [] }] };
7459
mockConfigExists.mockReturnValue(true);
7560
mockReadMcpSpec.mockResolvedValue(mockMcpSpec);
7661

77-
await createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW }));
78-
79-
const written = mockWriteMcpSpec.mock.calls[0]![0];
80-
expect(Array.isArray(written.unassignedTargets)).toBe(true);
62+
await expect(createExternalGatewayTarget(makeExternalConfig({ gateway: undefined }))).rejects.toThrow(
63+
'Gateway is required'
64+
);
8165
});
8266

8367
it('throws on duplicate target name in gateway', async () => {
@@ -92,19 +76,6 @@ describe('createExternalGatewayTarget', () => {
9276
);
9377
});
9478

95-
it('throws on duplicate target name in unassigned targets', async () => {
96-
const mockMcpSpec = {
97-
agentCoreGateways: [],
98-
unassignedTargets: [{ name: 'test-target' }],
99-
};
100-
mockConfigExists.mockReturnValue(true);
101-
mockReadMcpSpec.mockResolvedValue(mockMcpSpec);
102-
103-
await expect(createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW }))).rejects.toThrow(
104-
'Unassigned target "test-target" already exists'
105-
);
106-
});
107-
10879
it('throws when gateway not found', async () => {
10980
const mockMcpSpec = { agentCoreGateways: [] };
11081
mockConfigExists.mockReturnValue(true);

‎src/cli/operations/mcp/create-mcp.ts‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import {
1414
DEFAULT_HANDLER,
1515
DEFAULT_NODE_VERSION,
1616
DEFAULT_PYTHON_VERSION,
17-
SKIP_FOR_NOW,
1817
} from '../../tui/screens/mcp/types';
1918
import { existsSync } from 'fs';
2019
import { mkdir, readFile, writeFile } from 'fs/promises';
@@ -257,31 +256,22 @@ export async function createExternalGatewayTarget(config: AddGatewayTargetConfig
257256
...(config.outboundAuth && { outboundAuth: config.outboundAuth }),
258257
};
259258

260-
if (config.gateway && config.gateway !== SKIP_FOR_NOW) {
261-
// Assign to specific gateway
262-
const gateway = mcpSpec.agentCoreGateways.find(g => g.name === config.gateway);
263-
if (!gateway) {
264-
throw new Error(`Gateway "${config.gateway}" not found.`);
265-
}
266-
267-
// Check for duplicate target name
268-
if (gateway.targets.some(t => t.name === config.name)) {
269-
throw new Error(`Target "${config.name}" already exists in gateway "${gateway.name}".`);
270-
}
271-
272-
gateway.targets.push(target);
273-
} else {
274-
// Add to unassigned targets
275-
mcpSpec.unassignedTargets ??= [];
259+
if (!config.gateway) {
260+
throw new Error("Gateway is required. A gateway target must be attached to a gateway. Create a gateway first with 'agentcore add gateway'.");
261+
}
276262

277-
// Check for duplicate target name in unassigned targets
278-
if (mcpSpec.unassignedTargets.some((t: AgentCoreGatewayTarget) => t.name === config.name)) {
279-
throw new Error(`Unassigned target "${config.name}" already exists.`);
280-
}
263+
const gateway = mcpSpec.agentCoreGateways.find(g => g.name === config.gateway);
264+
if (!gateway) {
265+
throw new Error(`Gateway "${config.gateway}" not found.`);
266+
}
281267

282-
mcpSpec.unassignedTargets.push(target);
268+
// Check for duplicate target name
269+
if (gateway.targets.some(t => t.name === config.name)) {
270+
throw new Error(`Target "${config.name}" already exists in gateway "${gateway.name}".`);
283271
}
284272

273+
gateway.targets.push(target);
274+
285275
await configIO.writeMcpSpec(mcpSpec);
286276

287277
return { mcpDefsPath: '', toolName: config.name, projectPath: '' };

‎src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { useListNavigation } from '../../hooks';
66
import { generateUniqueName } from '../../utils';
77
import { useCreateIdentity, useExistingCredentialNames } from '../identity/useCreateIdentity.js';
88
import type { AddGatewayTargetConfig } from './types';
9-
import { MCP_TOOL_STEP_LABELS, OUTBOUND_AUTH_OPTIONS, SKIP_FOR_NOW } from './types';
9+
import { MCP_TOOL_STEP_LABELS, OUTBOUND_AUTH_OPTIONS } from './types';
1010
import { useAddGatewayTargetWizard } from './useAddGatewayTargetWizard';
1111
import { Box, Text } from 'ink';
1212
import React, { useMemo, useState } from 'react';
@@ -38,10 +38,7 @@ export function AddGatewayTargetScreen({
3838
const [apiKeyFields, setApiKeyFields] = useState({ name: '', apiKey: '' });
3939

4040
const gatewayItems: SelectableItem[] = useMemo(
41-
() => [
42-
...existingGateways.map(g => ({ id: g, title: g })),
43-
{ id: SKIP_FOR_NOW, title: 'Skip for now', description: 'Create unassigned target' },
44-
],
41+
() => existingGateways.map(g => ({ id: g, title: g })),
4542
[existingGateways]
4643
);
4744

@@ -388,8 +385,7 @@ export function AddGatewayTargetScreen({
388385
fields={[
389386
{ label: 'Name', value: wizard.config.name },
390387
...(wizard.config.endpoint ? [{ label: 'Endpoint', value: wizard.config.endpoint }] : []),
391-
...(wizard.config.gateway ? [{ label: 'Gateway', value: wizard.config.gateway }] : []),
392-
...(!wizard.config.gateway ? [{ label: 'Gateway', value: '(none - assign later)' }] : []),
388+
{ label: 'Gateway', value: wizard.config.gateway ?? '' },
393389
...(wizard.config.outboundAuth
394390
? [
395391
{ label: 'Auth Type', value: wizard.config.outboundAuth.type },

‎src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { APP_DIR, MCP_APP_SUBDIR } from '../../../../lib';
22
import type { ToolDefinition } from '../../../../schema';
33
import type { AddGatewayTargetConfig, AddGatewayTargetStep } from './types';
4-
import { SKIP_FOR_NOW } from './types';
54
import { useCallback, useMemo, useState } from 'react';
65

76
/**
@@ -66,10 +65,7 @@ export function useAddGatewayTargetWizard(existingGateways: string[] = []) {
6665
}, []);
6766

6867
const setGateway = useCallback((gateway: string) => {
69-
setConfig(c => {
70-
const isSkipped = gateway === SKIP_FOR_NOW;
71-
return { ...c, gateway: isSkipped ? undefined : gateway };
72-
});
68+
setConfig(c => ({ ...c, gateway }));
7369
setStep('outbound-auth');
7470
}, []);
7571

0 commit comments

Comments
 (0)