Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 55 additions & 12 deletions src/cli/commands/add/__tests__/validate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,21 @@ 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 {
readProjectSpec = mockReadProjectSpec;
},
}));

vi.mock('../../../operations/mcp/create-mcp.js', () => ({
getExistingGateways: (...args: unknown[]) => mockGetExistingGateways(...args),
}));

// Helper: valid base options for each type
const validAgentOptionsByo: AddAgentOptions = {
name: 'TestAgent',
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -331,6 +370,7 @@ describe('validate', () => {
const options: AddGatewayTargetOptions = {
name: 'test-tool',
language: 'Python',
gateway: 'my-gateway',
outboundAuthType: 'API_KEY',
credentialName: 'missing-cred',
};
Expand All @@ -347,6 +387,7 @@ describe('validate', () => {
const options: AddGatewayTargetOptions = {
name: 'test-tool',
language: 'Python',
gateway: 'my-gateway',
outboundAuthType: 'API_KEY',
credentialName: 'any-cred',
};
Expand All @@ -363,6 +404,7 @@ describe('validate', () => {
const options: AddGatewayTargetOptions = {
name: 'test-tool',
language: 'Python',
gateway: 'my-gateway',
outboundAuthType: 'API_KEY',
credentialName: 'valid-cred',
};
Expand Down Expand Up @@ -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);
Expand Down
26 changes: 25 additions & 1 deletion src/cli/commands/add/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
TargetLanguageSchema,
getSupportedModelProviders,
} from '../../../schema';
import { getExistingGateways } from '../../operations/mcp/create-mcp';
import type {
AddAgentOptions,
AddGatewayOptions,
Expand Down Expand Up @@ -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' };
Expand All @@ -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 };
}
Expand Down
39 changes: 5 additions & 34 deletions src/cli/operations/mcp/__tests__/create-mcp.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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);
Expand Down
42 changes: 15 additions & 27 deletions src/cli/operations/mcp/create-mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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: '' };
Expand Down
10 changes: 3 additions & 7 deletions src/cli/tui/screens/mcp/AddGatewayTargetScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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]
);

Expand Down Expand Up @@ -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 },
Expand Down
6 changes: 1 addition & 5 deletions src/cli/tui/screens/mcp/useAddGatewayTargetWizard.ts
Original file line number Diff line number Diff line change
@@ -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';

/**
Expand Down Expand Up @@ -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');
}, []);

Expand Down
Loading