From 515db0ec18c6276c288b5868cedc4a79c9e5aae4 Mon Sep 17 00:00:00 2001 From: notgitika Date: Sun, 21 Jun 2026 23:04:28 -0400 Subject: [PATCH] fix(run-insights): preserve TUI wizard state on error, validate name inline, surface job in history Multiple fixes to the `agentcore run insights` interactive wizard: - **Single-agent flow** (#4): when the project has exactly one agent, the wizard skipped the agent step but never set `config.agent`, so the Confirm screen showed a blank "Agent:" line. Pre-populate the sole agent in the default config so it's threaded through to confirm and the API call. - **Inline name validation** (#6): the Name step accepted invalid values (spaces, hyphens, leading digits) and only failed at submit time when the API returned a 400. Validate inline against the service-side `BatchEvaluationName` pattern (`^[a-zA-Z][a-zA-Z0-9_]{0,47}$`). Lookback days now also validate inline (1-90, integer only) instead of silently coercing 0 to 7. - **Preserve config on error** (#16): when the launch failed, hitting "Go back" reset the wizard to the Source step and discarded all selections. Now the failed config is stashed in flow state and the wizard re-opens at the Name step with everything pre-populated. - **Post-launch history** (#17): the wizard wrote to the job-engine store but `view insights` reads from the legacy `.cli/insights/*.json` store, so the brand-new job rendered as "No insights runs found". Mirror successful starts to the legacy store after launch. Bug bash items #4, #6, #16, #17 (mix of P1/P2). --- .../screens/run-insights/RunInsightsFlow.tsx | 80 ++++++++++++++++--- .../run-insights/RunInsightsScreen.tsx | 61 +++++++++++--- src/cli/tui/screens/run-insights/types.ts | 7 ++ .../run-insights/useRunInsightsWizard.ts | 42 ++++++---- 4 files changed, 155 insertions(+), 35 deletions(-) diff --git a/src/cli/tui/screens/run-insights/RunInsightsFlow.tsx b/src/cli/tui/screens/run-insights/RunInsightsFlow.tsx index a043f3552..ee5c96aed 100644 --- a/src/cli/tui/screens/run-insights/RunInsightsFlow.tsx +++ b/src/cli/tui/screens/run-insights/RunInsightsFlow.tsx @@ -1,20 +1,27 @@ import { ConfigIO } from '../../../../lib'; import type { DeployedState } from '../../../../schema'; +import { detectRegion } from '../../../aws/region'; import { getErrorMessage } from '../../../errors'; +import { saveInsightsRun } from '../../../operations/insights'; import { createJobEngine } from '../../../operations/jobs'; import type { InsightsJobRecord } from '../../../operations/jobs/shared/types'; import { withCommandRunTelemetry } from '../../../telemetry/cli-command-run.js'; import { ErrorPrompt, GradientText, SuccessPrompt } from '../../components'; import { RunInsightsScreen } from './RunInsightsScreen'; -import type { RunInsightsConfig } from './types'; +import type { RunInsightsConfig, RunInsightsStep } from './types'; import React, { useCallback, useEffect, useMemo, useState } from 'react'; +interface ProjectData { + agentNames: string[]; + onlineEvalConfigArns: string[]; +} + type FlowState = | { name: 'loading' } - | { name: 'wizard'; agentNames: string[]; onlineEvalConfigArns: string[] } + | { name: 'wizard'; project: ProjectData; resume?: { config: RunInsightsConfig; step: RunInsightsStep } } | { name: 'submitting' } | { name: 'success'; record: InsightsJobRecord } - | { name: 'error'; message: string }; + | { name: 'error'; message: string; project?: ProjectData; failedConfig?: RunInsightsConfig }; interface RunInsightsFlowProps { isInteractive?: boolean; @@ -51,7 +58,7 @@ export function RunInsightsFlow({ isInteractive = true, onExit, onBack, onViewJo const onlineEvalConfigArns = extractOnlineEvalConfigArns(deployedState); - setFlow({ name: 'wizard', agentNames, onlineEvalConfigArns }); + setFlow({ name: 'wizard', project: { agentNames, onlineEvalConfigArns } }); } catch (err) { if (!cancelled) setFlow({ name: 'error', message: getErrorMessage(err) }); } @@ -69,7 +76,7 @@ export function RunInsightsFlow({ isInteractive = true, onExit, onBack, onViewJo }, [isInteractive, flow.name, onExit]); const handleComplete = useCallback( - (config: RunInsightsConfig) => { + (config: RunInsightsConfig, project: ProjectData) => { setFlow({ name: 'submitting' }); void (async () => { @@ -88,9 +95,22 @@ export function RunInsightsFlow({ isInteractive = true, onExit, onBack, onViewJo if (!startResult.success) { throw startResult.error ?? new Error('Failed to start insights job'); } + + // Mirror the new job to the legacy insights store so `view insights` + // (InsightsJobsScreen) finds it. Without this, the post-launch screen + // shows "No insights runs found" right after creating the job. + await persistToLegacyStore(startResult.record).catch(() => { + // Non-fatal — the job started successfully; storage failures shouldn't surface. + }); + setFlow({ name: 'success', record: startResult.record }); } catch (err) { - setFlow({ name: 'error', message: getErrorMessage(err) }); + setFlow({ + name: 'error', + message: getErrorMessage(err), + project, + failedConfig: config, + }); } })(); }, @@ -104,10 +124,12 @@ export function RunInsightsFlow({ isInteractive = true, onExit, onBack, onViewJo if (flow.name === 'wizard') { return ( handleComplete(cfg, flow.project)} onExit={onBack} + initialConfig={flow.resume?.config} + initialStep={flow.resume?.step} /> ); } @@ -124,16 +146,54 @@ export function RunInsightsFlow({ isInteractive = true, onExit, onBack, onViewJo ); } + // Error: if we still have project data + the user's prior input, jump back + // into the wizard at the Name step with their config preserved. Otherwise + // (catastrophic load failure) fall back to the loading→error cycle. return ( setFlow({ name: 'loading' })} + onBack={() => { + if (flow.project && flow.failedConfig) { + setFlow({ + name: 'wizard', + project: flow.project, + resume: { config: flow.failedConfig, step: 'name' }, + }); + } else { + setFlow({ name: 'loading' }); + } + }} onExit={onExit} /> ); } +async function persistToLegacyStore(record: InsightsJobRecord): Promise { + // The job engine record uses `id`/`arn`; the legacy storage uses + // `batchEvaluationId`/`batchEvaluationArn`. Translate so InsightsJobsScreen + // (which reads from the legacy store) sees the new run. + const region = regionFromArn(record.arn) ?? (await detectRegion()).region; + saveInsightsRun({ + batchEvaluationId: record.id, + batchEvaluationArn: record.arn, + name: record.name, + status: record.status, + region, + createdAt: record.createdAt, + completedAt: record.completedAt, + insights: record.insights, + agent: record.agent, + }); +} + +function regionFromArn(arn: string | undefined): string | undefined { + if (!arn) return undefined; + const region = arn.split(':')[3]; + if (!region) return undefined; + return region; +} + function extractOnlineEvalConfigArns(deployedState: DeployedState): string[] { const arns: string[] = []; for (const target of Object.values(deployedState.targets ?? {})) { diff --git a/src/cli/tui/screens/run-insights/RunInsightsScreen.tsx b/src/cli/tui/screens/run-insights/RunInsightsScreen.tsx index 9384d83f9..94736cd57 100644 --- a/src/cli/tui/screens/run-insights/RunInsightsScreen.tsx +++ b/src/cli/tui/screens/run-insights/RunInsightsScreen.tsx @@ -10,20 +10,59 @@ import { import type { SelectableItem } from '../../components'; import { HELP_TEXT } from '../../constants'; import { useListNavigation, useMultiSelectNavigation } from '../../hooks'; -import { AVAILABLE_INSIGHTS, RUN_INSIGHTS_STEP_LABELS, SESSION_MODE_OPTIONS, SOURCE_OPTIONS } from './types'; -import type { RunInsightsConfig } from './types'; +import { + AVAILABLE_INSIGHTS, + JOB_NAME_PATTERN, + RUN_INSIGHTS_STEP_LABELS, + SESSION_MODE_OPTIONS, + SOURCE_OPTIONS, +} from './types'; +import type { RunInsightsConfig, RunInsightsStep } from './types'; import { useRunInsightsWizard } from './useRunInsightsWizard'; -import React, { useMemo } from 'react'; +import React, { forwardRef, useImperativeHandle, useMemo } from 'react'; + +export interface RunInsightsScreenHandle { + jumpToStep: (step: RunInsightsStep) => void; +} interface RunInsightsScreenProps { agentNames: string[]; onlineEvalConfigArns: string[]; onComplete: (config: RunInsightsConfig) => void; onExit: () => void; + /** Pre-seed wizard state, e.g. when returning from a validation/API error. */ + initialConfig?: RunInsightsConfig; + /** Step to land on at mount (default: 'source'). */ + initialStep?: RunInsightsStep; +} + +function validateJobName(value: string): true | string { + // Empty is allowed — the CLI auto-generates a name when omitted. + if (!value) return true; + if (!JOB_NAME_PATTERN.test(value)) { + return 'Name must start with a letter and contain only letters, numbers, and underscores (max 48 chars).'; + } + return true; +} + +function validateLookbackInput(value: string): true | string { + if (!value) return true; + const days = parseInt(value, 10); + if (!Number.isInteger(days) || String(days) !== value.trim()) { + return 'Lookback must be a whole number.'; + } + if (days < 1 || days > 90) { + return 'Lookback must be between 1 and 90 days.'; + } + return true; } -export function RunInsightsScreen({ agentNames, onlineEvalConfigArns, onComplete, onExit }: RunInsightsScreenProps) { - const wizard = useRunInsightsWizard(agentNames.length); +export const RunInsightsScreen = forwardRef(function RunInsightsScreen( + { agentNames, onlineEvalConfigArns, onComplete, onExit, initialConfig, initialStep }, + ref +) { + const wizard = useRunInsightsWizard(agentNames, initialConfig, initialStep); + useImperativeHandle(ref, () => ({ jumpToStep: wizard.jumpToStep }), [wizard.jumpToStep]); const isSourceStep = wizard.step === 'source'; const isAgentStep = wizard.step === 'agent'; @@ -157,10 +196,8 @@ export function RunInsightsScreen({ agentNames, onlineEvalConfigArns, onComplete key="lookback" prompt="Lookback window (days)" initialValue="7" - onSubmit={value => { - const days = parseInt(value, 10); - wizard.setLookbackDays(isNaN(days) || days <= 0 ? 7 : days); - }} + customValidation={validateLookbackInput} + onSubmit={value => wizard.setLookbackDays(parseInt(value, 10))} onCancel={() => wizard.goBack()} /> )} @@ -187,6 +224,8 @@ export function RunInsightsScreen({ agentNames, onlineEvalConfigArns, onComplete wizard.setName(value)} onCancel={() => wizard.goBack()} /> @@ -197,7 +236,7 @@ export function RunInsightsScreen({ agentNames, onlineEvalConfigArns, onComplete fields={ wizard.config.source === 'agent' ? [ - { label: 'Agent', value: wizard.config.agent ?? agentNames[0] ?? '' }, + { label: 'Agent', value: wizard.config.agent !== '' ? wizard.config.agent : (agentNames[0] ?? '') }, { label: 'Insights', value: wizard.config.insights @@ -218,4 +257,4 @@ export function RunInsightsScreen({ agentNames, onlineEvalConfigArns, onComplete ); -} +}); diff --git a/src/cli/tui/screens/run-insights/types.ts b/src/cli/tui/screens/run-insights/types.ts index 01dc0fd1b..c003e008b 100644 --- a/src/cli/tui/screens/run-insights/types.ts +++ b/src/cli/tui/screens/run-insights/types.ts @@ -36,6 +36,13 @@ export const RUN_INSIGHTS_STEP_LABELS: Record = { export const DEFAULT_LOOKBACK_DAYS = 7; +/** + * Validation pattern for job names. Mirrors the service-side BatchEvaluationName + * shape (`^[a-zA-Z][a-zA-Z0-9_]{0,47}$`) so we reject locally instead of + * surfacing a 400 from startBatchEvaluation. + */ +export const JOB_NAME_PATTERN = /^[a-zA-Z][a-zA-Z0-9_]{0,47}$/; + export const AVAILABLE_INSIGHTS = [ { id: 'Builtin.Insight.FailureAnalysis', diff --git a/src/cli/tui/screens/run-insights/useRunInsightsWizard.ts b/src/cli/tui/screens/run-insights/useRunInsightsWizard.ts index df2a16bc4..40cb1e8f6 100644 --- a/src/cli/tui/screens/run-insights/useRunInsightsWizard.ts +++ b/src/cli/tui/screens/run-insights/useRunInsightsWizard.ts @@ -12,10 +12,12 @@ function getStepsForSource(source: RunInsightsSource, agentCount: number): RunIn return ['source', 'agent', 'insights', 'sessions', 'lookbackDays', 'name', 'confirm']; } -function getDefaultConfig(): RunInsightsConfig { +function getDefaultConfig(soleAgent: string): RunInsightsConfig { return { source: 'agent', - agent: '', + // Pre-populate when only one agent exists so the confirm screen shows it + // even though the agent selection step is skipped. + agent: soleAgent, insights: [], sessionMode: 'lookback', lookbackDays: DEFAULT_LOOKBACK_DAYS, @@ -25,9 +27,15 @@ function getDefaultConfig(): RunInsightsConfig { }; } -export function useRunInsightsWizard(agentCount: number) { - const [config, setConfig] = useState(getDefaultConfig); - const [step, setStep] = useState('source'); +export function useRunInsightsWizard( + agentNames: string[], + initialConfig?: RunInsightsConfig, + initialStep: RunInsightsStep = 'source' +) { + const agentCount = agentNames.length; + const soleAgent = agentCount === 1 ? (agentNames[0] ?? '') : ''; + const [config, setConfig] = useState(() => initialConfig ?? getDefaultConfig(soleAgent)); + const [step, setStep] = useState(initialStep); const allSteps = useMemo(() => getStepsForSource(config.source, agentCount), [config.source, agentCount]); const currentIndex = allSteps.indexOf(step); @@ -50,6 +58,16 @@ export function useRunInsightsWizard(agentCount: number) { } }, [allSteps, currentIndex]); + /** Jump to an arbitrary step in the current flow without resetting config. */ + const jumpToStep = useCallback( + (target: RunInsightsStep) => { + if (allSteps.includes(target)) { + setStep(target); + } + }, + [allSteps] + ); + const setSource = useCallback( (source: RunInsightsSource) => { setConfig(c => ({ ...c, source })); @@ -80,13 +98,8 @@ export function useRunInsightsWizard(agentCount: number) { const setSessionMode = useCallback( (sessionMode: RunInsightsSessionMode) => { setConfig(c => ({ ...c, sessionMode })); - if (sessionMode === 'lookback') { - const next = nextStep('sessions'); - if (next) setStep(next); - } else { - const next = nextStep('sessions'); - if (next) setStep(next); - } + const next = nextStep('sessions'); + if (next) setStep(next); }, [nextStep] ); @@ -128,9 +141,9 @@ export function useRunInsightsWizard(agentCount: number) { ); const reset = useCallback(() => { - setConfig(getDefaultConfig()); + setConfig(getDefaultConfig(soleAgent)); setStep('source'); - }, []); + }, [soleAgent]); return { config, @@ -138,6 +151,7 @@ export function useRunInsightsWizard(agentCount: number) { steps: allSteps, currentIndex, goBack, + jumpToStep, setSource, setAgent, setInsights,