From 2b3859cb6cad9c8181425590c5cb1cc832e2e4cc Mon Sep 17 00:00:00 2001 From: loopy-symphony Date: Fri, 20 Mar 2026 18:07:50 +0000 Subject: [PATCH] fix: Fix validate command: unify CLI/TUI logic, add missing schema checks, improve TUI screen (#12) --- src/cli/commands/index.ts | 1 + .../validate/__tests__/action.test.ts | 171 +++++++++++++++++- src/cli/commands/validate/action.ts | 72 ++++++-- src/cli/commands/validate/command.tsx | 15 +- src/cli/commands/validate/index.ts | 2 +- .../tui/screens/validate/ValidateScreen.tsx | 151 +++++----------- 6 files changed, 286 insertions(+), 126 deletions(-) diff --git a/src/cli/commands/index.ts b/src/cli/commands/index.ts index 3dac1c82b..dc232396a 100644 --- a/src/cli/commands/index.ts +++ b/src/cli/commands/index.ts @@ -13,3 +13,4 @@ export { registerRun } from './run'; export { registerStatus } from './status'; export { registerTraces } from './traces'; export { registerUpdate } from './update'; +export { registerValidate } from './validate'; diff --git a/src/cli/commands/validate/__tests__/action.test.ts b/src/cli/commands/validate/__tests__/action.test.ts index aa4652ff7..3b996d88d 100644 --- a/src/cli/commands/validate/__tests__/action.test.ts +++ b/src/cli/commands/validate/__tests__/action.test.ts @@ -5,12 +5,16 @@ const { mockReadProjectSpec, mockReadAWSDeploymentTargets, mockReadDeployedState, + mockReadMcpSpec, + mockReadMcpDefs, mockConfigExists, mockFindConfigRoot, } = vi.hoisted(() => ({ mockReadProjectSpec: vi.fn(), mockReadAWSDeploymentTargets: vi.fn(), mockReadDeployedState: vi.fn(), + mockReadMcpSpec: vi.fn(), + mockReadMcpDefs: vi.fn(), mockConfigExists: vi.fn(), mockFindConfigRoot: vi.fn(), })); @@ -54,6 +58,8 @@ vi.mock('../../../../lib/index.js', () => { readProjectSpec = mockReadProjectSpec; readAWSDeploymentTargets = mockReadAWSDeploymentTargets; readDeployedState = mockReadDeployedState; + readMcpSpec = mockReadMcpSpec; + readMcpDefs = mockReadMcpDefs; configExists = mockConfigExists; }, ConfigValidationError, @@ -75,6 +81,7 @@ describe('handleValidate', () => { expect(result.success).toBe(false); expect(result.error).toContain('No agentcore project found'); + expect(result.results).toEqual([]); }); it('returns success when all configs are valid', async () => { @@ -86,22 +93,35 @@ describe('handleValidate', () => { const result = await handleValidate({}); expect(result.success).toBe(true); + expect(result.results).toHaveLength(5); + expect(result.results[0]).toEqual({ file: 'agentcore.json', success: true }); + expect(result.results[1]).toEqual({ file: 'aws-targets.json', success: true }); + // Optional files skipped + expect(result.results[2]).toEqual({ file: 'mcp.json', success: true, skipped: true }); + expect(result.results[3]).toEqual({ file: 'mcp-defs.json', success: true, skipped: true }); + expect(result.results[4]).toEqual({ file: '.cli/state.json', success: true, skipped: true }); }); it('returns error when project spec fails', async () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); mockReadProjectSpec.mockRejectedValue(new Error('invalid project')); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); expect(result.success).toBe(false); expect(result.error).toContain('invalid project'); + // Should still report results for all files + expect(result.results).toHaveLength(5); + expect(result.results[0]?.success).toBe(false); }); it('returns error when AWS targets fails', async () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); mockReadAWSDeploymentTargets.mockRejectedValue(new Error('bad targets')); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); @@ -113,7 +133,7 @@ describe('handleValidate', () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); mockReadAWSDeploymentTargets.mockResolvedValue([]); - mockConfigExists.mockReturnValue(true); + mockConfigExists.mockImplementation((type: string) => type === 'state'); mockReadDeployedState.mockResolvedValue({ targets: {} }); const result = await handleValidate({}); @@ -126,7 +146,7 @@ describe('handleValidate', () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); mockReadAWSDeploymentTargets.mockResolvedValue([]); - mockConfigExists.mockReturnValue(true); + mockConfigExists.mockImplementation((type: string) => type === 'state'); mockReadDeployedState.mockRejectedValue(new Error('bad state')); const result = await handleValidate({}); @@ -150,7 +170,11 @@ describe('handleValidate', () => { it('formats ConfigValidationError with its message', async () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); const { ConfigValidationError } = await import('../../../../lib/index.js'); - mockReadProjectSpec.mockRejectedValue(new (ConfigValidationError as any)('field "name" is required')); + const err = new Error('field "name" is required'); + Object.setPrototypeOf(err, ConfigValidationError.prototype); + mockReadProjectSpec.mockRejectedValue(err); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); @@ -162,6 +186,8 @@ describe('handleValidate', () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); const { ConfigParseError } = await import('../../../../lib/index.js'); mockReadProjectSpec.mockRejectedValue(new ConfigParseError('agentcore.json', new Error('Unexpected token'))); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); @@ -176,6 +202,8 @@ describe('handleValidate', () => { mockReadProjectSpec.mockRejectedValue( new ConfigReadError('agentcore.json', new Error('EACCES: permission denied')) ); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); @@ -188,6 +216,8 @@ describe('handleValidate', () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); const { ConfigNotFoundError } = await import('../../../../lib/index.js'); mockReadProjectSpec.mockRejectedValue(new ConfigNotFoundError('/path/agentcore.json', 'project')); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); @@ -198,10 +228,145 @@ describe('handleValidate', () => { it('formats non-Error values as strings', async () => { mockFindConfigRoot.mockReturnValue('/project/agentcore'); mockReadProjectSpec.mockRejectedValue('string error'); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); const result = await handleValidate({}); expect(result.success).toBe(false); expect(result.error).toBe('string error'); }); + + it('validates mcp.json when it exists', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockImplementation((type: string) => type === 'mcp'); + mockReadMcpSpec.mockResolvedValue({ mcpServers: {} }); + + const result = await handleValidate({}); + + expect(result.success).toBe(true); + expect(mockReadMcpSpec).toHaveBeenCalled(); + const mcpResult = result.results.find(r => r.file === 'mcp.json'); + expect(mcpResult).toEqual({ file: 'mcp.json', success: true }); + }); + + it('returns error when mcp.json is invalid', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockImplementation((type: string) => type === 'mcp'); + mockReadMcpSpec.mockRejectedValue(new Error('invalid mcp config')); + + const result = await handleValidate({}); + + expect(result.success).toBe(false); + expect(result.error).toContain('invalid mcp config'); + const mcpResult = result.results.find(r => r.file === 'mcp.json'); + expect(mcpResult?.success).toBe(false); + expect(mcpResult?.error).toContain('invalid mcp config'); + }); + + it('skips mcp.json when not present', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); + + const result = await handleValidate({}); + + expect(result.success).toBe(true); + expect(mockReadMcpSpec).not.toHaveBeenCalled(); + const mcpResult = result.results.find(r => r.file === 'mcp.json'); + expect(mcpResult).toEqual({ file: 'mcp.json', success: true, skipped: true }); + }); + + it('validates mcp-defs.json when it exists', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockImplementation((type: string) => type === 'mcpDefs'); + mockReadMcpDefs.mockResolvedValue({ tools: [] }); + + const result = await handleValidate({}); + + expect(result.success).toBe(true); + expect(mockReadMcpDefs).toHaveBeenCalled(); + const mcpDefsResult = result.results.find(r => r.file === 'mcp-defs.json'); + expect(mcpDefsResult).toEqual({ file: 'mcp-defs.json', success: true }); + }); + + it('returns error when mcp-defs.json is invalid', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockImplementation((type: string) => type === 'mcpDefs'); + mockReadMcpDefs.mockRejectedValue(new Error('invalid mcp definitions')); + + const result = await handleValidate({}); + + expect(result.success).toBe(false); + expect(result.error).toContain('invalid mcp definitions'); + const mcpDefsResult = result.results.find(r => r.file === 'mcp-defs.json'); + expect(mcpDefsResult?.success).toBe(false); + expect(mcpDefsResult?.error).toContain('invalid mcp definitions'); + }); + + it('skips mcp-defs.json when not present', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(false); + + const result = await handleValidate({}); + + expect(result.success).toBe(true); + expect(mockReadMcpDefs).not.toHaveBeenCalled(); + const mcpDefsResult = result.results.find(r => r.file === 'mcp-defs.json'); + expect(mcpDefsResult).toEqual({ file: 'mcp-defs.json', success: true, skipped: true }); + }); + + it('reports all errors instead of stopping on first failure', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockRejectedValue(new Error('bad project')); + mockReadAWSDeploymentTargets.mockRejectedValue(new Error('bad targets')); + mockConfigExists.mockReturnValue(true); + mockReadMcpSpec.mockRejectedValue(new Error('bad mcp')); + mockReadMcpDefs.mockRejectedValue(new Error('bad mcp defs')); + mockReadDeployedState.mockRejectedValue(new Error('bad state')); + + const result = await handleValidate({}); + + expect(result.success).toBe(false); + expect(result.results).toHaveLength(5); + // All files should have been validated (not stopped on first) + expect(result.results.every(r => !r.success)).toBe(true); + expect(result.error).toContain('bad project'); + expect(result.error).toContain('bad targets'); + expect(result.error).toContain('bad mcp'); + expect(result.error).toContain('bad mcp defs'); + expect(result.error).toContain('bad state'); + }); + + it('validates all 5 files when all exist and are valid', async () => { + mockFindConfigRoot.mockReturnValue('/project/agentcore'); + mockReadProjectSpec.mockResolvedValue({ name: 'Test', agents: [] }); + mockReadAWSDeploymentTargets.mockResolvedValue([]); + mockConfigExists.mockReturnValue(true); + mockReadMcpSpec.mockResolvedValue({ mcpServers: {} }); + mockReadMcpDefs.mockResolvedValue({ tools: [] }); + mockReadDeployedState.mockResolvedValue({ targets: {} }); + + const result = await handleValidate({}); + + expect(result.success).toBe(true); + expect(result.results).toHaveLength(5); + expect(result.results.every(r => r.success)).toBe(true); + expect(mockReadProjectSpec).toHaveBeenCalled(); + expect(mockReadAWSDeploymentTargets).toHaveBeenCalled(); + expect(mockReadMcpSpec).toHaveBeenCalled(); + expect(mockReadMcpDefs).toHaveBeenCalled(); + expect(mockReadDeployedState).toHaveBeenCalled(); + }); }); diff --git a/src/cli/commands/validate/action.ts b/src/cli/commands/validate/action.ts index 572f5cf7d..163675408 100644 --- a/src/cli/commands/validate/action.ts +++ b/src/cli/commands/validate/action.ts @@ -12,14 +12,35 @@ export interface ValidateOptions { directory?: string; } +export interface ValidateFileResult { + file: string; + success: boolean; + skipped?: boolean; + error?: string; +} + export interface ValidateResult { success: boolean; error?: string; + results: ValidateFileResult[]; } +/** + * Schema files validated by the validate command. + * Required files must exist; optional files are skipped when absent. + */ +const SCHEMA_FILES = [ + { key: 'project', label: 'agentcore.json', required: true }, + { key: 'targets', label: 'aws-targets.json', required: true }, + { key: 'mcp', label: 'mcp.json', required: false }, + { key: 'mcpDefs', label: 'mcp-defs.json', required: false }, + { key: 'state', label: '.cli/state.json', required: false }, +] as const; + /** * Validates all AgentCore schema files in the project. - * Returns a binary success/fail result with an error message if validation fails. + * Returns per-file results so both CLI and TUI can report granular status. + * All files are validated even if earlier ones fail. */ export async function handleValidate(options: ValidateOptions): Promise { const baseDir = options.directory ?? process.cwd(); @@ -30,35 +51,48 @@ export async function handleValidate(options: ValidateOptions): Promise !r.success); + const hasErrors = errors.length > 0; + + return { + success: !hasErrors, + error: hasErrors ? errors.map(r => r.error).join('\n') : undefined, + results, + }; } function formatError(err: unknown, fileName: string): string { diff --git a/src/cli/commands/validate/command.tsx b/src/cli/commands/validate/command.tsx index b4149a804..56e3fea9b 100644 --- a/src/cli/commands/validate/command.tsx +++ b/src/cli/commands/validate/command.tsx @@ -1,7 +1,7 @@ import { COMMAND_DESCRIPTIONS } from '../../tui/copy'; import { handleValidate } from './action'; import type { Command } from '@commander-js/extra-typings'; -import { Text, render } from 'ink'; +import { Box, Text, render } from 'ink'; export const registerValidate = (program: Command) => { program @@ -15,7 +15,18 @@ export const registerValidate = (program: Command) => { render(Valid); process.exit(0); } else { - render({result.error}); + render( + + {result.results + .filter(r => !r.success) + .map(r => ( + + {r.error} + + ))} + {result.results.length === 0 && result.error && {result.error}} + + ); process.exit(1); } }); diff --git a/src/cli/commands/validate/index.ts b/src/cli/commands/validate/index.ts index cecbb46ac..4a34d58e1 100644 --- a/src/cli/commands/validate/index.ts +++ b/src/cli/commands/validate/index.ts @@ -1,2 +1,2 @@ export { registerValidate } from './command'; -export { handleValidate, type ValidateOptions, type ValidateResult } from './action'; +export { handleValidate, type ValidateOptions, type ValidateResult, type ValidateFileResult } from './action'; diff --git a/src/cli/tui/screens/validate/ValidateScreen.tsx b/src/cli/tui/screens/validate/ValidateScreen.tsx index 24c0c743a..5420b76da 100644 --- a/src/cli/tui/screens/validate/ValidateScreen.tsx +++ b/src/cli/tui/screens/validate/ValidateScreen.tsx @@ -1,6 +1,7 @@ -import { ConfigIO, findConfigRoot } from '../../../../lib'; +import { handleValidate } from '../../../commands/validate/action'; +import type { ValidateFileResult } from '../../../commands/validate/action'; import { NextSteps, Screen, StepProgress } from '../../components'; -import type { Step } from '../../components'; +import type { NextStep, Step } from '../../components'; import { STATUS_COLORS } from '../../theme'; import { Box, Text } from 'ink'; import React, { useEffect, useState } from 'react'; @@ -15,124 +16,67 @@ type Phase = 'validating' | 'success' | 'error'; interface ValidationState { phase: Phase; steps: Step[]; - projectName: string | null; error: string | null; } -const SCHEMA_FILES = [ - { key: 'project', label: 'agentcore.json', required: true }, - { key: 'targets', label: 'aws-targets.json', required: true }, - { key: 'mcp', label: 'mcp.json', required: false }, - { key: 'mcpDefs', label: 'mcp-defs.json', required: false }, - { key: 'state', label: '.cli/state.json', required: false }, -] as const; +function getValidateNextSteps(success: boolean): NextStep[] { + if (success) { + return [ + { command: 'deploy', label: 'Deploy your agent' }, + { command: 'status', label: 'View deployment status' }, + ]; + } + return []; +} + +/** + * Maps handleValidate() results to StepProgress UI steps. + */ +function mapResultsToSteps(results: ValidateFileResult[]): Step[] { + return results.map(r => { + if (r.skipped) { + return { label: r.file, status: 'info' as const, info: 'Not present (optional)' }; + } + if (r.success) { + return { label: r.file, status: 'success' as const }; + } + return { label: r.file, status: 'error' as const, error: r.error }; + }); +} export function ValidateScreen({ isInteractive, onExit }: ValidateScreenProps) { const [state, setState] = useState({ phase: 'validating', - steps: SCHEMA_FILES.map(f => ({ label: f.label, status: 'pending' })), - projectName: null, + steps: [], error: null, }); useEffect(() => { const runValidation = async () => { - const configRoot = findConfigRoot(process.cwd()); - if (!configRoot) { - setState(prev => ({ - ...prev, + const result = await handleValidate({}); + + const steps = mapResultsToSteps(result.results); + + if (result.success) { + setState({ + phase: 'success', + steps, + error: null, + }); + } else { + setState({ phase: 'error', - error: 'No AgentCore project found in current directory', - steps: prev.steps.map((s, i) => - i === 0 ? { label: s.label, status: 'error', error: 'Project not found' } : s - ), - })); - return; - } - - const configIO = new ConfigIO({ baseDir: configRoot }); - let projectName: string | null = null; - - // Validate each file step by step - const newSteps: Step[] = SCHEMA_FILES.map(f => ({ label: f.label, status: 'pending' as const })); - - for (let i = 0; i < SCHEMA_FILES.length; i++) { - const file = SCHEMA_FILES[i]; - if (!file) continue; - - const currentStep = newSteps[i]; - if (!currentStep) continue; - - newSteps[i] = { label: currentStep.label, status: 'running' }; - setState(prev => ({ ...prev, steps: [...newSteps] })); - - // Small delay to show progress - await new Promise(resolve => setTimeout(resolve, 100)); - - try { - if (file.key === 'project') { - const spec = await configIO.readProjectSpec(); - projectName = spec.name; - newSteps[i] = { label: file.label, status: 'success' }; - } else if (file.key === 'targets') { - await configIO.readAWSDeploymentTargets(); - newSteps[i] = { label: file.label, status: 'success' }; - } else if (file.key === 'mcp') { - if (configIO.configExists('mcp')) { - await configIO.readMcpSpec(); - newSteps[i] = { label: file.label, status: 'success' }; - } else { - newSteps[i] = { label: file.label, status: 'info', info: 'Not present (optional)' }; - } - } else if (file.key === 'mcpDefs') { - if (configIO.configExists('mcpDefs')) { - await configIO.readMcpDefs(); - newSteps[i] = { label: file.label, status: 'success' }; - } else { - newSteps[i] = { label: file.label, status: 'info', info: 'Not present (optional)' }; - } - } else if (file.key === 'state') { - if (configIO.configExists('state')) { - await configIO.readDeployedState(); - newSteps[i] = { label: file.label, status: 'success' }; - } else { - newSteps[i] = { label: file.label, status: 'info', info: 'Not present (optional)' }; - } - } - setState(prev => ({ ...prev, steps: [...newSteps], projectName })); - } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err); - newSteps[i] = { label: file.label, status: 'error', error: errorMsg }; - setState({ - phase: 'error', - steps: [...newSteps], - projectName, - error: errorMsg, - }); - return; - } + steps: steps.length > 0 ? steps : [{ label: 'Project', status: 'error', error: result.error }], + error: result.error ?? 'Validation failed', + }); } - - setState({ - phase: 'success', - steps: newSteps, - projectName, - error: null, - }); }; void runValidation(); }, []); - const headerContent = state.projectName ? ( - - Project: - {state.projectName} - - ) : undefined; - return ( - + @@ -149,7 +93,12 @@ export function ValidateScreen({ isInteractive, onExit }: ValidateScreenProps) { )} {(state.phase === 'success' || state.phase === 'error') && ( - + )}