diff --git a/src/cli/commands/add/__tests__/validate.test.ts b/src/cli/commands/add/__tests__/validate.test.ts index 40f5dfec..3b319c9e 100644 --- a/src/cli/commands/add/__tests__/validate.test.ts +++ b/src/cli/commands/add/__tests__/validate.test.ts @@ -12,9 +12,10 @@ import { validateAddIdentityOptions, validateAddMemoryOptions, } from '../validate.js'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockReadProjectSpec = vi.fn(); +const mockGetExistingGateways = vi.fn(); vi.mock('../../../../lib/index.js', () => ({ ConfigIO: class { @@ -22,6 +23,10 @@ vi.mock('../../../../lib/index.js', () => ({ }, })); +vi.mock('../../../operations/mcp/create-mcp.js', () => ({ + getExistingGateways: (...args: unknown[]) => mockGetExistingGateways(...args), +})); + // Helper: valid base options for each type const validAgentOptionsByo: AddAgentOptions = { name: 'TestAgent', @@ -238,19 +243,48 @@ describe('validate', () => { }); describe('validateAddGatewayTargetOptions', () => { + beforeEach(() => { + // By default, mock that the gateway from validGatewayTargetOptions exists + mockGetExistingGateways.mockResolvedValue(['my-gateway']); + }); + // AC15: Required fields validated - it('returns error for missing required fields', async () => { - const requiredFields: { field: keyof AddGatewayTargetOptions; error: string }[] = [ - { field: 'name', error: '--name is required' }, - { field: 'language', error: '--language is required' }, - ]; + it('returns error for missing name', async () => { + const opts = { ...validGatewayTargetOptions, name: undefined }; + const result = await validateAddGatewayTargetOptions(opts); + expect(result.valid).toBe(false); + expect(result.error).toBe('--name is required'); + }); - for (const { field, error } of requiredFields) { - const opts = { ...validGatewayTargetOptions, [field]: undefined }; - const result = await validateAddGatewayTargetOptions(opts); - expect(result.valid, `Should fail for missing ${String(field)}`).toBe(false); - expect(result.error).toBe(error); - } + it('returns error for missing language (non-existing-endpoint)', async () => { + const opts = { ...validGatewayTargetOptions, language: undefined }; + const result = await validateAddGatewayTargetOptions(opts); + expect(result.valid).toBe(false); + expect(result.error).toBe('--language is required'); + }); + + // Gateway is required + it('returns error when --gateway is missing', async () => { + const opts = { ...validGatewayTargetOptions, gateway: undefined }; + const result = await validateAddGatewayTargetOptions(opts); + expect(result.valid).toBe(false); + expect(result.error).toContain('--gateway is required'); + }); + + it('returns error when no gateways exist', async () => { + mockGetExistingGateways.mockResolvedValue([]); + const result = await validateAddGatewayTargetOptions(validGatewayTargetOptions); + expect(result.valid).toBe(false); + expect(result.error).toContain('No gateways found'); + expect(result.error).toContain('agentcore add gateway'); + }); + + it('returns error when specified gateway does not exist', async () => { + mockGetExistingGateways.mockResolvedValue(['other-gateway']); + const result = await validateAddGatewayTargetOptions(validGatewayTargetOptions); + expect(result.valid).toBe(false); + expect(result.error).toContain('Gateway "my-gateway" not found'); + expect(result.error).toContain('other-gateway'); }); // AC16: Invalid values rejected @@ -274,6 +308,7 @@ describe('validate', () => { name: 'test-tool', source: 'existing-endpoint', endpoint: 'https://example.com/mcp', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(true); @@ -285,6 +320,7 @@ describe('validate', () => { name: 'test-tool', source: 'existing-endpoint', endpoint: 'http://localhost:3000/mcp', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(true); @@ -294,6 +330,7 @@ describe('validate', () => { const options: AddGatewayTargetOptions = { name: 'test-tool', source: 'existing-endpoint', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(false); @@ -305,6 +342,7 @@ describe('validate', () => { name: 'test-tool', source: 'existing-endpoint', endpoint: 'ftp://example.com/mcp', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(false); @@ -316,6 +354,7 @@ describe('validate', () => { name: 'test-tool', source: 'existing-endpoint', endpoint: 'not-a-url', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(false); @@ -331,6 +370,7 @@ describe('validate', () => { const options: AddGatewayTargetOptions = { name: 'test-tool', language: 'Python', + gateway: 'my-gateway', outboundAuthType: 'API_KEY', credentialName: 'missing-cred', }; @@ -347,6 +387,7 @@ describe('validate', () => { const options: AddGatewayTargetOptions = { name: 'test-tool', language: 'Python', + gateway: 'my-gateway', outboundAuthType: 'API_KEY', credentialName: 'any-cred', }; @@ -363,6 +404,7 @@ describe('validate', () => { const options: AddGatewayTargetOptions = { name: 'test-tool', language: 'Python', + gateway: 'my-gateway', outboundAuthType: 'API_KEY', credentialName: 'valid-cred', }; @@ -429,6 +471,7 @@ describe('validate', () => { source: 'existing-endpoint', endpoint: 'https://example.com/mcp', host: 'Lambda', + gateway: 'my-gateway', }; const result = await validateAddGatewayTargetOptions(options); expect(result.valid).toBe(false); diff --git a/src/cli/commands/add/validate.ts b/src/cli/commands/add/validate.ts index b11e2aa7..9a4bc4df 100644 --- a/src/cli/commands/add/validate.ts +++ b/src/cli/commands/add/validate.ts @@ -8,6 +8,7 @@ import { TargetLanguageSchema, getSupportedModelProviders, } from '../../../schema'; +import { getExistingGateways } from '../../operations/mcp/create-mcp'; import type { AddAgentOptions, AddGatewayOptions, @@ -197,6 +198,30 @@ export async function validateAddGatewayTargetOptions(options: AddGatewayTargetO return { valid: false, error: 'Invalid source. Valid options: existing-endpoint, create-new' }; } + // Gateway is required — a gateway target must be attached to a gateway + if (!options.gateway) { + return { + valid: false, + error: + "--gateway is required. A gateway target must be attached to a gateway. Create a gateway first with 'agentcore add gateway'.", + }; + } + + // Validate the specified gateway exists + const existingGateways = await getExistingGateways(); + if (existingGateways.length === 0) { + return { + valid: false, + error: "No gateways found. Create a gateway first with 'agentcore add gateway' before adding a gateway target.", + }; + } + if (!existingGateways.includes(options.gateway)) { + return { + valid: false, + error: `Gateway "${options.gateway}" not found. Available gateways: ${existingGateways.join(', ')}`, + }; + } + if (options.source === 'existing-endpoint') { if (options.host) { return { valid: false, error: '--host is not applicable for existing endpoint targets' }; @@ -216,7 +241,6 @@ export async function validateAddGatewayTargetOptions(options: AddGatewayTargetO // Populate defaults for fields skipped by external endpoint flow options.language ??= 'Other'; - options.gateway ??= undefined; return { valid: true }; } diff --git a/src/cli/operations/mcp/__tests__/create-mcp.test.ts b/src/cli/operations/mcp/__tests__/create-mcp.test.ts index 398e8f32..25e5f173 100644 --- a/src/cli/operations/mcp/__tests__/create-mcp.test.ts +++ b/src/cli/operations/mcp/__tests__/create-mcp.test.ts @@ -1,4 +1,3 @@ -import { SKIP_FOR_NOW } from '../../../tui/screens/mcp/types.js'; import type { AddGatewayConfig, AddGatewayTargetConfig } from '../../../tui/screens/mcp/types.js'; import { createExternalGatewayTarget, createGatewayFromWizard, getUnassignedTargets } from '../create-mcp.js'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -55,29 +54,14 @@ describe('createExternalGatewayTarget', () => { expect(gateway.targets[0]!.targetType).toBe('mcpServer'); }); - it('stores target in unassignedTargets when gateway is skip-for-now', async () => { - const mockMcpSpec = { agentCoreGateways: [] }; - mockConfigExists.mockReturnValue(true); - mockReadMcpSpec.mockResolvedValue(mockMcpSpec); - - await createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW })); - - expect(mockWriteMcpSpec).toHaveBeenCalled(); - const written = mockWriteMcpSpec.mock.calls[0]![0]; - expect(written.unassignedTargets).toHaveLength(1); - expect(written.unassignedTargets[0]!.name).toBe('test-target'); - expect(written.unassignedTargets[0]!.endpoint).toBe('https://api.example.com'); - }); - - it('initializes unassignedTargets array if it does not exist in mcp spec', async () => { - const mockMcpSpec = { agentCoreGateways: [] }; + it('throws when gateway is not provided', async () => { + const mockMcpSpec = { agentCoreGateways: [{ name: 'test-gateway', targets: [] }] }; mockConfigExists.mockReturnValue(true); mockReadMcpSpec.mockResolvedValue(mockMcpSpec); - await createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW })); - - const written = mockWriteMcpSpec.mock.calls[0]![0]; - expect(Array.isArray(written.unassignedTargets)).toBe(true); + await expect(createExternalGatewayTarget(makeExternalConfig({ gateway: undefined }))).rejects.toThrow( + 'Gateway is required' + ); }); it('throws on duplicate target name in gateway', async () => { @@ -92,19 +76,6 @@ describe('createExternalGatewayTarget', () => { ); }); - it('throws on duplicate target name in unassigned targets', async () => { - const mockMcpSpec = { - agentCoreGateways: [], - unassignedTargets: [{ name: 'test-target' }], - }; - mockConfigExists.mockReturnValue(true); - mockReadMcpSpec.mockResolvedValue(mockMcpSpec); - - await expect(createExternalGatewayTarget(makeExternalConfig({ gateway: SKIP_FOR_NOW }))).rejects.toThrow( - 'Unassigned target "test-target" already exists' - ); - }); - it('throws when gateway not found', async () => { const mockMcpSpec = { agentCoreGateways: [] }; mockConfigExists.mockReturnValue(true); diff --git a/src/cli/operations/mcp/create-mcp.ts b/src/cli/operations/mcp/create-mcp.ts index aefd731c..1f554642 100644 --- a/src/cli/operations/mcp/create-mcp.ts +++ b/src/cli/operations/mcp/create-mcp.ts @@ -10,12 +10,7 @@ import type { import { AgentCoreCliMcpDefsSchema, ToolDefinitionSchema } from '../../../schema'; import { getTemplateToolDefinitions, renderGatewayTargetTemplate } from '../../templates/GatewayTargetRenderer'; import type { AddGatewayConfig, AddGatewayTargetConfig } from '../../tui/screens/mcp/types'; -import { - DEFAULT_HANDLER, - DEFAULT_NODE_VERSION, - DEFAULT_PYTHON_VERSION, - SKIP_FOR_NOW, -} from '../../tui/screens/mcp/types'; +import { DEFAULT_HANDLER, DEFAULT_NODE_VERSION, DEFAULT_PYTHON_VERSION } from '../../tui/screens/mcp/types'; import { existsSync } from 'fs'; import { mkdir, readFile, writeFile } from 'fs/promises'; import { dirname, join } from 'path'; @@ -257,31 +252,24 @@ export async function createExternalGatewayTarget(config: AddGatewayTargetConfig ...(config.outboundAuth && { outboundAuth: config.outboundAuth }), }; - if (config.gateway && config.gateway !== SKIP_FOR_NOW) { - // Assign to specific gateway - const gateway = mcpSpec.agentCoreGateways.find(g => g.name === config.gateway); - if (!gateway) { - throw new Error(`Gateway "${config.gateway}" not found.`); - } - - // Check for duplicate target name - if (gateway.targets.some(t => t.name === config.name)) { - throw new Error(`Target "${config.name}" already exists in gateway "${gateway.name}".`); - } - - gateway.targets.push(target); - } else { - // Add to unassigned targets - mcpSpec.unassignedTargets ??= []; + if (!config.gateway) { + throw new Error( + "Gateway is required. A gateway target must be attached to a gateway. Create a gateway first with 'agentcore add gateway'." + ); + } - // Check for duplicate target name in unassigned targets - if (mcpSpec.unassignedTargets.some((t: AgentCoreGatewayTarget) => t.name === config.name)) { - throw new Error(`Unassigned target "${config.name}" already exists.`); - } + const gateway = mcpSpec.agentCoreGateways.find(g => g.name === config.gateway); + if (!gateway) { + throw new Error(`Gateway "${config.gateway}" not found.`); + } - mcpSpec.unassignedTargets.push(target); + // Check for duplicate target name + if (gateway.targets.some(t => t.name === config.name)) { + throw new Error(`Target "${config.name}" already exists in gateway "${gateway.name}".`); } + gateway.targets.push(target); + await configIO.writeMcpSpec(mcpSpec); return { mcpDefsPath: '', toolName: config.name, projectPath: '' }; diff --git a/src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx b/src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx index 30ece187..1dd507ae 100644 --- a/src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx +++ b/src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx @@ -6,7 +6,7 @@ import { useListNavigation } from '../../hooks'; import { generateUniqueName } from '../../utils'; import { useCreateIdentity, useExistingCredentialNames } from '../identity/useCreateIdentity.js'; import type { AddGatewayTargetConfig } from './types'; -import { MCP_TOOL_STEP_LABELS, OUTBOUND_AUTH_OPTIONS, SKIP_FOR_NOW } from './types'; +import { MCP_TOOL_STEP_LABELS, OUTBOUND_AUTH_OPTIONS } from './types'; import { useAddGatewayTargetWizard } from './useAddGatewayTargetWizard'; import { Box, Text } from 'ink'; import React, { useMemo, useState } from 'react'; @@ -38,10 +38,7 @@ export function AddGatewayTargetScreen({ const [apiKeyFields, setApiKeyFields] = useState({ name: '', apiKey: '' }); const gatewayItems: SelectableItem[] = useMemo( - () => [ - ...existingGateways.map(g => ({ id: g, title: g })), - { id: SKIP_FOR_NOW, title: 'Skip for now', description: 'Create unassigned target' }, - ], + () => existingGateways.map(g => ({ id: g, title: g })), [existingGateways] ); @@ -388,8 +385,7 @@ export function AddGatewayTargetScreen({ fields={[ { label: 'Name', value: wizard.config.name }, ...(wizard.config.endpoint ? [{ label: 'Endpoint', value: wizard.config.endpoint }] : []), - ...(wizard.config.gateway ? [{ label: 'Gateway', value: wizard.config.gateway }] : []), - ...(!wizard.config.gateway ? [{ label: 'Gateway', value: '(none - assign later)' }] : []), + { label: 'Gateway', value: wizard.config.gateway ?? '' }, ...(wizard.config.outboundAuth ? [ { label: 'Auth Type', value: wizard.config.outboundAuth.type }, diff --git a/src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts b/src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts index 71a96fe6..d29daaf6 100644 --- a/src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts +++ b/src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts @@ -1,7 +1,6 @@ import { APP_DIR, MCP_APP_SUBDIR } from '../../../../lib'; import type { ToolDefinition } from '../../../../schema'; import type { AddGatewayTargetConfig, AddGatewayTargetStep } from './types'; -import { SKIP_FOR_NOW } from './types'; import { useCallback, useMemo, useState } from 'react'; /** @@ -66,10 +65,7 @@ export function useAddGatewayTargetWizard(existingGateways: string[] = []) { }, []); const setGateway = useCallback((gateway: string) => { - setConfig(c => { - const isSkipped = gateway === SKIP_FOR_NOW; - return { ...c, gateway: isSkipped ? undefined : gateway }; - }); + setConfig(c => ({ ...c, gateway })); setStep('outbound-auth'); }, []);