From 12821fc5038a1404d2e918593af51b9aaf7dd144 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Thu, 16 Jul 2026 19:44:56 +0000 Subject: [PATCH 1/8] feat: manage vended CDK project dependencies with minor-version pinning Pin managed dependencies in the vended agentcore/cdk/package.json to tilde ranges (exact for @aws/agentcore-cdk), sync them to the CLI's tested versions during deploy preflight, auto-migrate pre-pinning projects, and error with an upgrade hint when the project was updated by a newer CLI. Opt-out via 'agentcore config disableDependencyManagement true'. --- .../assets.snapshot.test.ts.snap | 18 +- src/assets/cdk/package.json | 18 +- src/cli/commands/deploy/actions.ts | 19 +++ src/cli/commands/deploy/command.tsx | 11 +- src/cli/commands/deploy/types.ts | 2 + src/cli/operations/deploy/dependency-sync.ts | 25 +++ src/cli/operations/deploy/index.ts | 3 + src/cli/telemetry/schemas/command-run.ts | 5 + src/cli/telemetry/schemas/common-shapes.ts | 2 + src/cli/tui/hooks/useCdkPreflight.ts | 36 +++- src/cli/tui/screens/deploy/DeployScreen.tsx | 8 + src/cli/tui/screens/deploy/useDeployFlow.ts | 45 ++++- .../__tests__/policy.test.ts | 159 ++++++++++++++++++ .../__tests__/semver.test.ts | 80 +++++++++ .../__tests__/sync.test.ts | 153 +++++++++++++++++ src/lib/dependency-management/index.ts | 112 ++++++++++++ src/lib/dependency-management/messages.ts | 68 ++++++++ src/lib/dependency-management/policy.ts | 100 +++++++++++ src/lib/dependency-management/semver.ts | 96 +++++++++++ src/lib/dependency-management/types.ts | 54 ++++++ src/lib/errors/types.ts | 21 +++ src/lib/schemas/io/global-config.ts | 1 + 22 files changed, 1007 insertions(+), 29 deletions(-) create mode 100644 src/cli/operations/deploy/dependency-sync.ts create mode 100644 src/lib/dependency-management/__tests__/policy.test.ts create mode 100644 src/lib/dependency-management/__tests__/semver.test.ts create mode 100644 src/lib/dependency-management/__tests__/sync.test.ts create mode 100644 src/lib/dependency-management/index.ts create mode 100644 src/lib/dependency-management/messages.ts create mode 100644 src/lib/dependency-management/policy.ts create mode 100644 src/lib/dependency-management/semver.ts create mode 100644 src/lib/dependency-management/types.ts diff --git a/src/assets/__tests__/__snapshots__/assets.snapshot.test.ts.snap b/src/assets/__tests__/__snapshots__/assets.snapshot.test.ts.snap index b935e4f72..ca6a42bc3 100644 --- a/src/assets/__tests__/__snapshots__/assets.snapshot.test.ts.snap +++ b/src/assets/__tests__/__snapshots__/assets.snapshot.test.ts.snap @@ -636,18 +636,18 @@ exports[`Assets Directory Snapshots > CDK assets > cdk/cdk/package.json should m "format:check": "prettier --check ." }, "devDependencies": { - "@types/jest": "^29.5.14", - "@types/node": "^24.10.1", - "jest": "^29.7.0", - "ts-jest": "^29.2.5", - "aws-cdk": "2.1126.0", - "prettier": "^3.4.2", + "@types/jest": "~29.5.14", + "@types/node": "~24.13.3", + "jest": "~29.7.0", + "ts-jest": "~29.4.11", + "aws-cdk": "~2.1126.0", + "prettier": "~3.9.5", "typescript": "~5.9.3" }, "dependencies": { - "@aws/agentcore-cdk": "^0.1.0-alpha.19", - "aws-cdk-lib": "^2.248.0", - "constructs": "^10.0.0" + "@aws/agentcore-cdk": "0.1.0-alpha.45", + "aws-cdk-lib": "~2.261.0", + "constructs": "~10.7.0" } } " diff --git a/src/assets/cdk/package.json b/src/assets/cdk/package.json index 4646bfb3e..550a52797 100644 --- a/src/assets/cdk/package.json +++ b/src/assets/cdk/package.json @@ -14,17 +14,17 @@ "format:check": "prettier --check ." }, "devDependencies": { - "@types/jest": "^29.5.14", - "@types/node": "^24.10.1", - "jest": "^29.7.0", - "ts-jest": "^29.2.5", - "aws-cdk": "2.1126.0", - "prettier": "^3.4.2", + "@types/jest": "~29.5.14", + "@types/node": "~24.13.3", + "jest": "~29.7.0", + "ts-jest": "~29.4.11", + "aws-cdk": "~2.1126.0", + "prettier": "~3.9.5", "typescript": "~5.9.3" }, "dependencies": { - "@aws/agentcore-cdk": "^0.1.0-alpha.19", - "aws-cdk-lib": "^2.248.0", - "constructs": "^10.0.0" + "@aws/agentcore-cdk": "0.1.0-alpha.45", + "aws-cdk-lib": "~2.261.0", + "constructs": "~10.7.0" } } diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 841355934..3509109fc 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -33,6 +33,7 @@ import { checkBootstrapNeeded, checkStackDeployability, ensureDefaultDeploymentTarget, + ensureManagedDependencies, getAllCredentials, hasIdentityApiProviders, hasIdentityOAuthProviders, @@ -261,6 +262,20 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0 ? allWarnings : undefined, + dependencySync: depSync, }; } catch (err: unknown) { logger.log(getErrorMessage(err), 'error'); diff --git a/src/cli/commands/deploy/command.tsx b/src/cli/commands/deploy/command.tsx index 0af261125..e5511863c 100644 --- a/src/cli/commands/deploy/command.tsx +++ b/src/cli/commands/deploy/command.tsx @@ -42,10 +42,19 @@ async function handleDeployCLI(options: DeployOptions): Promise { .then(spec => computeDeployAttrs(spec, mode)) .catch(() => ({ ...DEFAULT_DEPLOY_ATTRS, mode }) as const); - const { deployResult } = await withCommandRunTelemetry('deploy', attrs, async () => { + const { deployResult } = await withCommandRunTelemetry('deploy', attrs, async recorder => { const result = await executeDeploy(options).catch( (e): DeployResult => ({ success: false, error: e instanceof Error ? e : new Error(getErrorMessage(e)) }) ); + if (result.success && result.dependencySync) { + recorder.set({ + dep_sync_changed_count: result.dependencySync.changes.length + result.dependencySync.restored.length, + dep_sync_migrated: result.dependencySync.migrated, + dep_sync_opted_out: result.dependencySync.optedOut, + dep_sync_skew_warning: result.dependencySync.skewWarning, + dep_sync_reinstalled: result.dependencySync.reinstalled, + }); + } if (!result.success) { return { success: false as const, error: result.error, deployResult: result }; } diff --git a/src/cli/commands/deploy/types.ts b/src/cli/commands/deploy/types.ts index a29b6ef8e..ec066de4e 100644 --- a/src/cli/commands/deploy/types.ts +++ b/src/cli/commands/deploy/types.ts @@ -1,3 +1,4 @@ +import type { DependencySyncResult } from '../../../lib/dependency-management'; import type { Result } from '../../../lib/result'; export interface DeployOptions { @@ -17,6 +18,7 @@ export type DeployResult = Result<{ nextSteps?: string[]; notes?: string[]; postDeployWarnings?: string[]; + dependencySync?: DependencySyncResult; }> & { logPath?: string }; export type PreflightResult = Result<{ diff --git a/src/cli/operations/deploy/dependency-sync.ts b/src/cli/operations/deploy/dependency-sync.ts new file mode 100644 index 000000000..a54b6999a --- /dev/null +++ b/src/cli/operations/deploy/dependency-sync.ts @@ -0,0 +1,25 @@ +import { syncManagedDependencies } from '../../../lib/dependency-management'; +import type { DependencySyncResult } from '../../../lib/dependency-management'; +import { readGlobalConfigSync } from '../../../lib/schemas/io/global-config'; +import type { LocalCdkProject } from '../../cdk/local-cdk-project'; +import { getTemplatePath } from '../../templates/templateRoot'; + +/** + * Deploy-preflight entry point for managed dependency pinning (#1540): resolves the + * CLI's vended CDK package.json (source of truth) and the global opt-out, then runs + * the sync against the project's agentcore/cdk directory. A future `agentcore build` + * command should call this same function. + * + * Throws CliVersionTooOldError when the project was updated by a newer CLI, and + * DependencySyncError on rewrite/reinstall failure. + */ +export async function ensureManagedDependencies(cdkProject: LocalCdkProject): Promise { + const disabled = readGlobalConfigSync().disableDependencyManagement === true; + return syncManagedDependencies({ + vendedPackageJsonPath: getTemplatePath('cdk', 'package.json'), + projectDir: cdkProject.projectDir, + disabled, + }); +} + +export type { DependencySyncResult }; diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index d4eb1e5df..62b3e2701 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -68,6 +68,9 @@ export { ensureDefaultDeploymentTarget } from './ensure-target'; // Pre-synth backfill of vpcId for pre-existing Container+VPC configs written before vpcId was added export { backfillContainerVpcIds, type BackfillVpcIdResult } from './backfill-vpc-id'; +// Managed dependency pinning (#1540) — shared by CLI deploy + TUI preflight +export { ensureManagedDependencies, type DependencySyncResult } from './dependency-sync'; + // Managed-memory heads-up (shared by the CLI command + TUI deploy flow + add harness) export { MANAGED_MEMORY_DEPLOY_NOTICE, diff --git a/src/cli/telemetry/schemas/command-run.ts b/src/cli/telemetry/schemas/command-run.ts index f92b15afc..42094b092 100644 --- a/src/cli/telemetry/schemas/command-run.ts +++ b/src/cli/telemetry/schemas/command-run.ts @@ -119,6 +119,11 @@ const DeployAttrs = safeSchema({ policy_engine_count: Count, policy_count: Count, deploy_mode: DeployModeSchema, + dep_sync_changed_count: Count.optional(), + dep_sync_migrated: z.boolean().optional(), + dep_sync_opted_out: z.boolean().optional(), + dep_sync_skew_warning: z.boolean().optional(), + dep_sync_reinstalled: z.boolean().optional(), }); const DevAttrs = safeSchema({ diff --git a/src/cli/telemetry/schemas/common-shapes.ts b/src/cli/telemetry/schemas/common-shapes.ts index 6072b87ad..255268bee 100644 --- a/src/cli/telemetry/schemas/common-shapes.ts +++ b/src/cli/telemetry/schemas/common-shapes.ts @@ -110,8 +110,10 @@ export const ErrorName = z.enum([ 'ConfigReadError', 'ConfigValidationError', 'ConfigWriteError', + 'CliVersionTooOldError', 'ConflictError', 'DependencyCheckError', + 'DependencySyncError', 'DevServerConnectionError', 'DevServerError', 'GitInitError', diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index b2462130e..6000dba61 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -1,4 +1,5 @@ import { ConfigIO, SecureCredentials, toError } from '../../../lib'; +import type { DependencySyncResult } from '../../../lib/dependency-management'; import { AwsCredentialsError, UserCancellationError } from '../../../lib/errors/types'; import type { DeployedState } from '../../../schema'; import { applyTargetRegionToEnv } from '../../aws'; @@ -14,6 +15,7 @@ import { checkBootstrapNeeded, checkDependencyVersions, checkStackDeployability, + ensureManagedDependencies, formatError, getAllCredentials, hasIdentityApiProviders, @@ -162,6 +164,8 @@ export interface PreflightResult { missingCredentials: MissingCredential[]; /** KMS key ARN used for identity token vault encryption */ identityKmsKeyArn?: string; + /** Managed dependency sync outcome (#1540) — notice/warnings for display, attrs for telemetry */ + dependencySync: DependencySyncResult | null; /** Credential ARNs (API key + OAuth) from pre-deploy setup */ allCredentials: Record; startPreflight: () => Promise; @@ -184,13 +188,15 @@ export interface PreflightResult { // Step indices for base preflight steps (always present) const STEP_VALIDATE = 0; const STEP_DEPS = 1; -const STEP_BUILD = 2; -// Note: Identity steps are inserted at index 3+ when needed, shifting synth and stack status down. +const STEP_SYNC_DEPS = 2; +const STEP_BUILD = 3; +// Note: Identity steps are inserted at index 4+ when needed, shifting synth and stack status down. // Use findStepIndex() to locate synth and stack status dynamically. const BASE_PREFLIGHT_STEPS: Step[] = [ { label: 'Validate project', status: 'pending' }, { label: 'Check dependencies', status: 'pending' }, + { label: 'Sync CDK dependencies', status: 'pending' }, { label: 'Build CDK project', status: 'pending' }, { label: 'Synthesize CloudFormation', status: 'pending' }, { label: 'Check stack status', status: 'pending' }, @@ -226,6 +232,7 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { Record >({}); const [teardownConfirmed, setTeardownConfirmed] = useState(false); + const [dependencySync, setDependencySync] = useState(null); const lastErrorRef = useRef(undefined); // Guard against concurrent runs (React StrictMode, re-renders, etc.) @@ -485,6 +492,30 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { return; } + // Step: Sync managed dependencies (#1540) — pin agentcore/cdk/package.json to the versions + // this CLI was tested with, migrating pre-pinning (caret) projects. Runs before the build so + // the compile sees the reinstalled tree. + updateStep(STEP_SYNC_DEPS, { status: 'running' }); + logger.startStep('Sync CDK dependencies'); + try { + const syncResult = await ensureManagedDependencies(preflightContext.cdkProject); + for (const warning of syncResult.warnings) { + logger.log(warning, 'warn'); + } + if (syncResult.notice) { + logger.log(syncResult.notice); + } + setDependencySync(syncResult); + logger.endStep('success'); + updateStep(STEP_SYNC_DEPS, { status: 'success' }); + } catch (err) { + const errorMsg = formatError(err); + logger.endStep('error', errorMsg); + updateStep(STEP_SYNC_DEPS, { status: 'error', error: getErrorMessage(err) }); + failPreflight(err); + return; + } + // Step: Build CDK project updateStep(STEP_BUILD, { status: 'running' }); logger.startStep('Build CDK project'); @@ -1047,6 +1078,7 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { lastError: lastErrorRef.current, missingCredentials, identityKmsKeyArn, + dependencySync, allCredentials, startPreflight, confirmTeardown, diff --git a/src/cli/tui/screens/deploy/DeployScreen.tsx b/src/cli/tui/screens/deploy/DeployScreen.tsx index e033bb9fa..3abe0e9aa 100644 --- a/src/cli/tui/screens/deploy/DeployScreen.tsx +++ b/src/cli/tui/screens/deploy/DeployScreen.tsx @@ -83,6 +83,7 @@ export function DeployScreen({ numStacksWithChanges, deployNotes, managedMemoryNotice, + dependencySyncNotice, postDeployWarnings, postDeployHasError, isDiffLoading, @@ -344,6 +345,13 @@ export function DeployScreen({ <> + {/* Managed dependency sync summary (#1540): what preflight changed in agentcore/cdk/package.json. */} + {dependencySyncNotice && ( + + {dependencySyncNotice} + + )} + {/* Managed-memory heads-up: shown while the slow CFN apply runs (not gated on success). Styled as a plain dim "Note:" to match the transaction-search note convention below. */} {managedMemoryNotice && !isComplete && ( diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index c866b924c..c585bf009 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -1,4 +1,5 @@ import { ConfigIO } from '../../../../lib'; +import type { DependencySyncResult } from '../../../../lib/dependency-management'; import type { AwsDeploymentTarget } from '../../../../schema'; import type { CdkToolkitWrapper, DeployMessage, SwitchableIoHost } from '../../../cdk/toolkit-lib'; import { @@ -115,6 +116,8 @@ interface DeployFlowState { deployNotes: string[]; /** Managed-memory heads-up, shown while the CFN apply runs (null when not applicable) */ managedMemoryNotice: string | null; + /** Managed dependency sync summary from preflight (#1540), null when nothing changed */ + dependencySyncNotice: string | null; /** Warnings from post-deploy steps (config bundles, AB tests) */ postDeployWarnings: string[]; /** True if any post-deploy sub-resource operation had errors */ @@ -140,6 +143,19 @@ interface DeployFlowState { skipCredentials: () => void; } +/** Overlay dep_sync_* telemetry attrs from the preflight dependency sync (#1540), if it ran. */ +function withDepSyncAttrs(attrs: T, sync: DependencySyncResult | null): T { + if (!sync) return attrs; + return { + ...attrs, + dep_sync_changed_count: sync.changes.length + sync.restored.length, + dep_sync_migrated: sync.migrated, + dep_sync_opted_out: sync.optedOut, + dep_sync_skew_warning: sync.skewWarning, + dep_sync_reinstalled: sync.reinstalled, + }; +} + export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState { const { preSynthesized, isInteractive = false, diffMode = false, selectedTargets } = options; const skipPreflight = !!preSynthesized; @@ -741,7 +757,10 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState // Preflight failed — emit telemetry and bail if (preflight.phase === 'error') { const error = preflight.lastError ?? new Error('Preflight failed'); - const attrs = context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }; + const attrs = withDepSyncAttrs( + context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }, + preflight.dependencySync + ); withCommandRunTelemetry('deploy', attrs, () => ({ success: false as const, error })).catch(() => { /* telemetry is best-effort */ }); @@ -751,7 +770,10 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState if (deployStep.status !== 'pending') return; if (!cdkToolkitWrapper) return; - const attrs = context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }; + const attrs = withDepSyncAttrs( + context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }, + preflight.dependencySync + ); const run = async (): Promise<{ success: true } | { success: false; error: Error }> => { // Run diff before deploy to capture pre-deploy differences. @@ -1022,9 +1044,12 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState // Preflight failed — emit telemetry and bail if (preflight.phase === 'error') { const error = preflight.lastError ?? new Error('Preflight failed'); - const attrs = context - ? computeDeployAttrs(context.projectSpec, 'diff') - : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }; + const attrs = withDepSyncAttrs( + context + ? computeDeployAttrs(context.projectSpec, 'diff') + : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }, + preflight.dependencySync + ); withCommandRunTelemetry('deploy', attrs, () => ({ success: false as const, error })).catch(() => { /* telemetry is best-effort */ }); @@ -1034,9 +1059,12 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState if (diffStep.status !== 'pending') return; if (!cdkToolkitWrapper) return; - const attrs = context - ? computeDeployAttrs(context.projectSpec, 'diff') - : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }; + const attrs = withDepSyncAttrs( + context + ? computeDeployAttrs(context.projectSpec, 'diff') + : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }, + preflight.dependencySync + ); const run = async (): Promise<{ success: true } | { success: false; error: Error }> => { setDiffStep(prev => ({ ...prev, status: 'running' })); @@ -1233,6 +1261,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState numStacksWithChanges, deployNotes, managedMemoryNotice, + dependencySyncNotice: preflight.dependencySync?.notice ?? null, postDeployWarnings, postDeployHasError, isDiffLoading, diff --git a/src/lib/dependency-management/__tests__/policy.test.ts b/src/lib/dependency-management/__tests__/policy.test.ts new file mode 100644 index 000000000..06af5dad5 --- /dev/null +++ b/src/lib/dependency-management/__tests__/policy.test.ts @@ -0,0 +1,159 @@ +import { computeSyncPlan } from '../policy'; +import type { PackageManifest } from '../types'; +import { describe, expect, it } from 'vitest'; + +const VENDED: PackageManifest = { + dependencies: { + '@aws/agentcore-cdk': '0.1.0-alpha.45', + 'aws-cdk-lib': '~2.261.0', + constructs: '~10.7.0', + }, + devDependencies: { + 'aws-cdk': '~2.1126.0', + typescript: '~5.9.3', + }, +}; + +function project(overrides: Partial = {}): PackageManifest { + return { + dependencies: { ...VENDED.dependencies }, + devDependencies: { ...VENDED.devDependencies }, + ...overrides, + }; +} + +describe('computeSyncPlan', () => { + it('produces an empty plan when the project already matches', () => { + const plan = computeSyncPlan(VENDED, project()); + expect(plan).toEqual({ skew: [], changes: [], restored: [], skipped: [], migratedFromCaret: false }); + }); + + it('migrates caret ranges to the vended pin and flags the migration', () => { + const plan = computeSyncPlan( + VENDED, + project({ + dependencies: { + '@aws/agentcore-cdk': '^0.1.0-alpha.19', + 'aws-cdk-lib': '^2.248.0', + constructs: '^10.0.0', + }, + }) + ); + expect(plan.migratedFromCaret).toBe(true); + expect(plan.skew).toEqual([]); + expect(plan.changes).toEqual([ + { name: '@aws/agentcore-cdk', section: 'dependencies', from: '^0.1.0-alpha.19', to: '0.1.0-alpha.45' }, + { name: 'aws-cdk-lib', section: 'dependencies', from: '^2.248.0', to: '~2.261.0' }, + { name: 'constructs', section: 'dependencies', from: '^10.0.0', to: '~10.7.0' }, + ]); + }); + + it('syncs older tilde ranges without flagging migration', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.250.0' } }) + ); + expect(plan.migratedFromCaret).toBe(false); + expect(plan.changes).toEqual([{ name: 'aws-cdk-lib', section: 'dependencies', from: '~2.250.0', to: '~2.261.0' }]); + }); + + it('flags skew when the project declares a higher minor', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.290.0' } }) + ); + expect(plan.skew).toEqual([{ name: 'aws-cdk-lib', declared: '~2.290.0', expected: '~2.261.0' }]); + expect(plan.changes).toEqual([]); + }); + + it('flags skew when the project declares a higher major', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~3.0.0' } }) + ); + expect(plan.skew).toHaveLength(1); + }); + + it('treats a higher patch as syncable, not skew', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.261.5' } }) + ); + expect(plan.skew).toEqual([]); + expect(plan.changes).toEqual([{ name: 'aws-cdk-lib', section: 'dependencies', from: '~2.261.5', to: '~2.261.0' }]); + }); + + it('detects prerelease skew on the exact-pinned construct (alpha.50 > alpha.45)', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, '@aws/agentcore-cdk': '0.1.0-alpha.50' } }) + ); + expect(plan.skew).toEqual([{ name: '@aws/agentcore-cdk', declared: '0.1.0-alpha.50', expected: '0.1.0-alpha.45' }]); + }); + + it('upgrades an older exact-pinned prerelease', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, '@aws/agentcore-cdk': '0.1.0-alpha.19' } }) + ); + expect(plan.skew).toEqual([]); + expect(plan.changes).toEqual([ + { name: '@aws/agentcore-cdk', section: 'dependencies', from: '0.1.0-alpha.19', to: '0.1.0-alpha.45' }, + ]); + }); + + it('never touches user-added dependencies', () => { + const plan = computeSyncPlan( + VENDED, + project({ dependencies: { ...VENDED.dependencies, lodash: '^4.17.21', 'left-pad': '*' } }) + ); + expect(plan.changes).toEqual([]); + expect(plan.skipped).toEqual([]); + }); + + it('restores a deleted managed dependency', () => { + const proj = project(); + delete proj.dependencies!.constructs; + const plan = computeSyncPlan(VENDED, proj); + expect(plan.restored).toEqual([{ name: 'constructs', section: 'dependencies', to: '~10.7.0' }]); + }); + + it('skips managed deps with non-semver specifiers (bundled tarball override)', () => { + const plan = computeSyncPlan( + VENDED, + project({ + dependencies: { ...VENDED.dependencies, '@aws/agentcore-cdk': 'file:bundled-agentcore-cdk.tgz' }, + }) + ); + expect(plan.changes).toEqual([]); + expect(plan.skew).toEqual([]); + expect(plan.skipped).toEqual([ + { + name: '@aws/agentcore-cdk', + raw: 'file:bundled-agentcore-cdk.tgz', + reason: 'uses a non-semver specifier and was left unmanaged', + }, + ]); + }); + + it('manages devDependencies and respects the section the user has the dep in', () => { + const proj = project({ + dependencies: { ...VENDED.dependencies, 'aws-cdk': '~2.1000.0' }, + devDependencies: { typescript: '~5.8.0' }, + }); + delete proj.devDependencies!['aws-cdk']; + const plan = computeSyncPlan(VENDED, proj); + expect(plan.changes).toContainEqual({ + name: 'aws-cdk', + section: 'dependencies', + from: '~2.1000.0', + to: '~2.1126.0', + }); + expect(plan.changes).toContainEqual({ + name: 'typescript', + section: 'devDependencies', + from: '~5.8.0', + to: '~5.9.3', + }); + }); +}); diff --git a/src/lib/dependency-management/__tests__/semver.test.ts b/src/lib/dependency-management/__tests__/semver.test.ts new file mode 100644 index 000000000..9450055d5 --- /dev/null +++ b/src/lib/dependency-management/__tests__/semver.test.ts @@ -0,0 +1,80 @@ +import { compareVersions, formatVersion, parseSpecifier, parseVersion } from '../semver'; +import { describe, expect, it } from 'vitest'; + +describe('parseVersion', () => { + it('parses release versions', () => { + expect(parseVersion('2.261.0')).toEqual({ major: 2, minor: 261, patch: 0, prerelease: [] }); + }); + + it('parses prerelease versions with numeric identifiers', () => { + expect(parseVersion('0.1.0-alpha.19')).toEqual({ major: 0, minor: 1, patch: 0, prerelease: ['alpha', 19] }); + }); + + it('ignores build metadata', () => { + expect(parseVersion('1.2.3+build.5')).toEqual({ major: 1, minor: 2, patch: 3, prerelease: [] }); + }); + + it('rejects non-versions', () => { + expect(parseVersion('latest')).toBeNull(); + expect(parseVersion('1.2')).toBeNull(); + expect(parseVersion('')).toBeNull(); + }); +}); + +describe('compareVersions', () => { + const v = (s: string) => parseVersion(s)!; + + it('orders by major, minor, patch', () => { + expect(compareVersions(v('1.0.0'), v('2.0.0'))).toBeLessThan(0); + expect(compareVersions(v('2.1.0'), v('2.0.9'))).toBeGreaterThan(0); + expect(compareVersions(v('2.0.1'), v('2.0.1'))).toBe(0); + }); + + it('orders prerelease identifiers numerically (alpha.19 < alpha.20)', () => { + expect(compareVersions(v('0.1.0-alpha.19'), v('0.1.0-alpha.20'))).toBeLessThan(0); + expect(compareVersions(v('0.1.0-alpha.20'), v('0.1.0-alpha.19'))).toBeGreaterThan(0); + }); + + it('ranks prerelease below its release (0.1.0-alpha.45 < 0.1.0)', () => { + expect(compareVersions(v('0.1.0-alpha.45'), v('0.1.0'))).toBeLessThan(0); + }); + + it('ranks numeric prerelease identifiers below alphanumeric', () => { + expect(compareVersions(v('1.0.0-1'), v('1.0.0-alpha'))).toBeLessThan(0); + }); + + it('ranks a shorter prerelease set below a longer one', () => { + expect(compareVersions(v('1.0.0-alpha'), v('1.0.0-alpha.1'))).toBeLessThan(0); + }); +}); + +describe('parseSpecifier', () => { + it('classifies exact, tilde, and caret', () => { + expect(parseSpecifier('2.1126.0').kind).toBe('exact'); + expect(parseSpecifier('~2.261.0').kind).toBe('tilde'); + expect(parseSpecifier('^0.1.0-alpha.19').kind).toBe('caret'); + expect(parseSpecifier('=1.2.3').kind).toBe('exact'); + }); + + it('marks non-semver specifiers unsupported', () => { + for (const raw of [ + 'file:bundled-agentcore-cdk.tgz', + 'git+https://github.com/aws/agentcore-cdk.git', + 'workspace:*', + '*', + 'latest', + '>=1.0.0 <2.0.0', + '1.x', + 'https://example.com/pkg.tgz', + ]) { + expect(parseSpecifier(raw)).toEqual({ kind: 'unsupported', raw }); + } + }); +}); + +describe('formatVersion', () => { + it('round-trips release and prerelease versions', () => { + expect(formatVersion(parseVersion('2.261.0')!)).toBe('2.261.0'); + expect(formatVersion(parseVersion('0.1.0-alpha.45')!)).toBe('0.1.0-alpha.45'); + }); +}); diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts new file mode 100644 index 000000000..08e0ab4c6 --- /dev/null +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -0,0 +1,153 @@ +import { CliVersionTooOldError, DependencySyncError } from '../../errors/types'; +import { syncManagedDependencies } from '../index'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockRunSubprocessCapture = vi.hoisted(() => vi.fn()); +vi.mock('../../utils/subprocess', () => ({ + runSubprocessCapture: mockRunSubprocessCapture, +})); + +const VENDED = { + name: 'agentcore-cdk-app', + dependencies: { + '@aws/agentcore-cdk': '0.1.0-alpha.45', + 'aws-cdk-lib': '~2.261.0', + }, + devDependencies: { + typescript: '~5.9.3', + }, +}; + +describe('syncManagedDependencies', () => { + let tempDir: string; + let projectDir: string; + let vendedPath: string; + + const writeProject = (manifest: object) => { + writeFileSync(path.join(projectDir, 'package.json'), JSON.stringify(manifest, null, 2) + '\n'); + }; + const readProject = () => JSON.parse(readFileSync(path.join(projectDir, 'package.json'), 'utf-8')); + const run = (disabled = false) => + syncManagedDependencies({ vendedPackageJsonPath: vendedPath, projectDir, disabled }); + + beforeEach(() => { + tempDir = mkdtempSync(path.join(tmpdir(), 'dep-sync-test-')); + projectDir = path.join(tempDir, 'agentcore', 'cdk'); + mkdirSync(projectDir, { recursive: true }); + vendedPath = path.join(tempDir, 'vended-package.json'); + writeFileSync(vendedPath, JSON.stringify(VENDED, null, 2)); + mockRunSubprocessCapture.mockResolvedValue({ stdout: '', stderr: '', code: 0, signal: null }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + vi.clearAllMocks(); + }); + + it('is a no-op when the project already matches (no write, no reinstall, no notice)', async () => { + writeProject({ dependencies: { ...VENDED.dependencies }, devDependencies: { ...VENDED.devDependencies } }); + const result = await run(); + expect(result.changes).toEqual([]); + expect(result.reinstalled).toBe(false); + expect(result.notice).toBeNull(); + expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); + }); + + it('migrates caret ranges, reinstalls, and reports', async () => { + mkdirSync(path.join(projectDir, 'node_modules'), { recursive: true }); + writeFileSync(path.join(projectDir, 'package-lock.json'), '{}'); + writeProject({ + dependencies: { '@aws/agentcore-cdk': '^0.1.0-alpha.19', 'aws-cdk-lib': '^2.248.0', lodash: '^4.17.21' }, + devDependencies: { typescript: '~5.9.3' }, + }); + + const result = await run(); + + expect(result.migrated).toBe(true); + expect(result.reinstalled).toBe(true); + expect(result.changes).toHaveLength(2); + expect(result.notice).toContain('created before the AgentCore CLI managed dependency versions'); + expect(result.notice).toContain('Dependencies you added yourself were not changed.'); + expect(result.notice).toContain('disableDependencyManagement'); + + const written = readProject(); + expect(written.dependencies['@aws/agentcore-cdk']).toBe('0.1.0-alpha.45'); + expect(written.dependencies['aws-cdk-lib']).toBe('~2.261.0'); + expect(written.dependencies.lodash).toBe('^4.17.21'); + + expect(existsSync(path.join(projectDir, 'node_modules'))).toBe(false); + expect(existsSync(path.join(projectDir, 'package-lock.json'))).toBe(false); + expect(mockRunSubprocessCapture).toHaveBeenCalledWith('npm', ['install'], { cwd: projectDir }); + }); + + it('preserves key order and unknown fields when rewriting', async () => { + writeProject({ + zeta: 'kept', + dependencies: { lodash: '1.0.0', 'aws-cdk-lib': '~2.250.0', '@aws/agentcore-cdk': '0.1.0-alpha.45' }, + scripts: { build: 'tsc' }, + devDependencies: { typescript: '~5.9.3' }, + }); + await run(); + const raw = readFileSync(path.join(projectDir, 'package.json'), 'utf-8'); + const written = JSON.parse(raw); + expect(Object.keys(written)).toEqual(['zeta', 'dependencies', 'scripts', 'devDependencies']); + expect(Object.keys(written.dependencies)).toEqual(['lodash', 'aws-cdk-lib', '@aws/agentcore-cdk']); + expect(written.zeta).toBe('kept'); + }); + + it('throws CliVersionTooOldError when the project declares a higher minor', async () => { + writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0' }, devDependencies: {} }); + await expect(run()).rejects.toThrow(CliVersionTooOldError); + await expect(run()).rejects.toThrow(/newer version of the AgentCore CLI/); + }); + + it('downgrades skew to a warning and touches nothing when disabled', async () => { + const manifest = { + dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0', old: '^1.0.0' }, + devDependencies: { typescript: '~5.8.0' }, + }; + writeProject(manifest); + const result = await run(true); + expect(result.optedOut).toBe(true); + expect(result.skewWarning).toBe(true); + expect(result.warnings.some(w => w.includes('newer than this CLI was tested with'))).toBe(true); + expect(result.notice).toBeNull(); + expect(readProject()).toEqual(manifest); + expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); + }); + + it('restores a deleted managed dependency', async () => { + writeProject({ + dependencies: { '@aws/agentcore-cdk': '0.1.0-alpha.45' }, + devDependencies: { typescript: '~5.9.3' }, + }); + const result = await run(); + expect(result.restored).toEqual([{ name: 'aws-cdk-lib', section: 'dependencies', to: '~2.261.0' }]); + expect(readProject().dependencies['aws-cdk-lib']).toBe('~2.261.0'); + expect(result.notice).toContain('aws-cdk-lib'); + }); + + it('skips a file: override with a warning and no rewrite', async () => { + writeProject({ + dependencies: { ...VENDED.dependencies, '@aws/agentcore-cdk': 'file:bundled-agentcore-cdk.tgz' }, + devDependencies: { ...VENDED.devDependencies }, + }); + const result = await run(); + expect(result.skipped).toHaveLength(1); + expect(result.warnings[0]).toContain('file:bundled-agentcore-cdk.tgz'); + expect(readProject().dependencies['@aws/agentcore-cdk']).toBe('file:bundled-agentcore-cdk.tgz'); + }); + + it('wraps npm install failure in DependencySyncError', async () => { + mockRunSubprocessCapture.mockResolvedValue({ stdout: '', stderr: 'E404', code: 1, signal: null }); + writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.250.0' }, devDependencies: {} }); + await expect(run()).rejects.toThrow(DependencySyncError); + }); + + it('wraps unreadable project package.json in DependencySyncError', async () => { + await expect(run()).rejects.toThrow(DependencySyncError); + }); +}); diff --git a/src/lib/dependency-management/index.ts b/src/lib/dependency-management/index.ts new file mode 100644 index 000000000..51f4e7ad6 --- /dev/null +++ b/src/lib/dependency-management/index.ts @@ -0,0 +1,112 @@ +import { CliVersionTooOldError, DependencySyncError } from '../errors/types'; +import { runSubprocessCapture } from '../utils/subprocess'; +import { CLI_UPGRADE_ERROR_MESSAGE, formatSkewWarning, formatSkippedWarning, formatSyncNotice } from './messages'; +import { computeSyncPlan } from './policy'; +import type { DependencySyncResult, PackageManifest, SyncManagedDependenciesOptions } from './types'; +import { readFile, rm, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; + +export type { + DependencyChange, + DependencySyncResult, + RestoredDependency, + SkippedDependency, + SyncManagedDependenciesOptions, +} from './types'; + +async function readManifest(path: string, what: string): Promise { + let raw: string; + try { + raw = await readFile(path, 'utf-8'); + } catch (err) { + throw new DependencySyncError(`Could not read ${what} at ${path}: ${String(err)}`, { cause: err }); + } + try { + return JSON.parse(raw) as PackageManifest; + } catch (err) { + throw new DependencySyncError(`Could not parse ${what} at ${path}: ${String(err)}`, { cause: err }); + } +} + +/** + * Sync the managed dependencies of a vended CDK project to the versions this CLI + * release was tested with (agentcore-cli#1540). + * + * The vended asset package.json is the source of truth: every dependency named in it + * is managed and rewritten to its vended specifier (tilde for stable deps so patches + * float, exact for the L3 constructs); everything else in the user's file is untouched. + * Projects that predate pinning (caret ranges) are migrated automatically. If the + * project declares a managed dependency newer than this CLI expects, the project was + * updated by a newer CLI and this throws CliVersionTooOldError (downgraded to a + * warning in the result when `disabled`). When anything changed, node_modules and the + * lockfile are deleted and dependencies reinstalled so the installed tree matches. + * + * All user-facing wording is returned on the result (`notice`, `warnings`) — callers + * only display it. Throws CliVersionTooOldError | DependencySyncError; never exits. + */ +export async function syncManagedDependencies(options: SyncManagedDependenciesOptions): Promise { + const { vendedPackageJsonPath, projectDir, disabled = false } = options; + const projectPackageJsonPath = join(projectDir, 'package.json'); + + const vended = await readManifest(vendedPackageJsonPath, 'the vended CDK package.json'); + const project = await readManifest(projectPackageJsonPath, "the project's CDK package.json"); + + const plan = computeSyncPlan(vended, project); + + const result: DependencySyncResult = { + optedOut: disabled, + migrated: plan.migratedFromCaret, + reinstalled: false, + skewWarning: false, + changes: plan.changes, + restored: plan.restored, + skipped: plan.skipped, + warnings: plan.skipped.map(formatSkippedWarning), + notice: null, + }; + + if (plan.skew.length > 0) { + if (!disabled) { + throw new CliVersionTooOldError(CLI_UPGRADE_ERROR_MESSAGE); + } + result.skewWarning = true; + result.warnings.push(formatSkewWarning(plan.skew)); + } + + if (disabled || (plan.changes.length === 0 && plan.restored.length === 0)) { + return result; + } + + // Mutate the parsed manifest in place so user key order and unknown fields survive. + for (const change of plan.changes) { + project[change.section]![change.name] = change.to; + } + for (const restoredDep of plan.restored) { + const section = (project[restoredDep.section] ??= {}); + section[restoredDep.name] = restoredDep.to; + } + + try { + await writeFile(projectPackageJsonPath, JSON.stringify(project, null, 2) + '\n', 'utf-8'); + await rm(join(projectDir, 'node_modules'), { recursive: true, force: true }); + await rm(join(projectDir, 'package-lock.json'), { force: true }); + } catch (err) { + throw new DependencySyncError(`Failed to update the CDK project's dependencies: ${String(err)}`, { cause: err }); + } + + const install = await runSubprocessCapture('npm', ['install'], { cwd: projectDir }); + if (install.code !== 0) { + throw new DependencySyncError( + `npm install failed after updating managed dependencies (exit ${String(install.code)}): ${install.stderr}` + ); + } + result.reinstalled = true; + + result.notice = formatSyncNotice({ + migrated: result.migrated, + changes: result.changes, + restored: result.restored, + reinstalled: result.reinstalled, + }); + return result; +} diff --git a/src/lib/dependency-management/messages.ts b/src/lib/dependency-management/messages.ts new file mode 100644 index 000000000..f72c1cc11 --- /dev/null +++ b/src/lib/dependency-management/messages.ts @@ -0,0 +1,68 @@ +import type { SkewFinding } from './policy'; +import type { DependencyChange, RestoredDependency, SkippedDependency } from './types'; + +/** + * Single source of user-facing wording for dependency management, so the + * headless and TUI deploy paths (and any future CLI) can't drift apart. + */ + +export const CLI_UPGRADE_ERROR_MESSAGE = + 'This project requires a newer version of the AgentCore CLI. ' + + 'Run `npm install -g @aws/agentcore-cli@latest` and retry.'; + +export const OPT_OUT_HINT = 'To manage these versions yourself: agentcore config disableDependencyManagement true'; + +const MIGRATION_PREAMBLE = + 'Your project was created before the AgentCore CLI managed dependency versions.\n' + + "We've updated agentcore/cdk/package.json so the CLI keeps these dependencies\n" + + 'on versions it has been tested with (patch updates still apply automatically):'; + +function formatChangeTable(changes: DependencyChange[], restored: RestoredDependency[]): string { + const rows: { name: string; from: string; to: string }[] = [ + ...changes.map(c => ({ name: c.name, from: c.from, to: c.to })), + ...restored.map(r => ({ name: r.name, from: '(removed)', to: r.to })), + ]; + const nameWidth = Math.max(...rows.map(r => r.name.length)); + const fromWidth = Math.max(...rows.map(r => r.from.length)); + return rows.map(r => ` ${r.name.padEnd(nameWidth)} ${r.from.padEnd(fromWidth)} → ${r.to}`).join('\n'); +} + +export function formatSyncNotice(options: { + migrated: boolean; + changes: DependencyChange[]; + restored: RestoredDependency[]; + reinstalled: boolean; +}): string | null { + const { migrated, changes, restored, reinstalled } = options; + if (changes.length === 0 && restored.length === 0) return null; + + const lines: string[] = []; + if (migrated) { + lines.push(MIGRATION_PREAMBLE); + } else { + lines.push('Updated managed dependencies in agentcore/cdk/package.json:'); + } + lines.push(''); + lines.push(formatChangeTable(changes, restored)); + lines.push(''); + if (reinstalled) { + lines.push('Reinstalled agentcore/cdk dependencies.'); + } + lines.push('Dependencies you added yourself were not changed.'); + if (migrated) { + lines.push(OPT_OUT_HINT); + } + return lines.join('\n'); +} + +export function formatSkewWarning(skew: SkewFinding[]): string { + const deps = skew.map(s => `${s.name} (${s.declared}, CLI expects ${s.expected})`).join(', '); + return ( + `${deps} ${skew.length === 1 ? 'is' : 'are'} newer than this CLI was tested with; ` + + 'deploy may fail — upgrade the CLI with `npm install -g @aws/agentcore-cli@latest`.' + ); +} + +export function formatSkippedWarning(skipped: SkippedDependency): string { + return `${skipped.name} (${skipped.raw}) ${skipped.reason}.`; +} diff --git a/src/lib/dependency-management/policy.ts b/src/lib/dependency-management/policy.ts new file mode 100644 index 000000000..1fe330aa8 --- /dev/null +++ b/src/lib/dependency-management/policy.ts @@ -0,0 +1,100 @@ +import { compareVersions, parseSpecifier } from './semver'; +import type { ParsedSpecifier } from './semver'; +import type { + DependencyChange, + DependencySection, + PackageManifest, + RestoredDependency, + SkippedDependency, +} from './types'; + +/** + * Pure policy computation for managed-dependency syncing. No I/O. + * + * A dependency is "managed" iff its name appears in the vended manifest + * (dependencies or devDependencies). The vended specifier is authoritative: + * the sync copies it verbatim, so the pinning policy (tilde for stable deps, + * exact for L3 constructs) lives entirely in the vended asset. + */ + +export interface SkewFinding { + name: string; + declared: string; + expected: string; +} + +export interface SyncPlan { + /** Managed deps where the project declares a newer minor/major than the CLI expects. */ + skew: SkewFinding[]; + changes: DependencyChange[]; + restored: RestoredDependency[]; + skipped: SkippedDependency[]; + /** True when any managed dep had a caret range — the project predates pinning. */ + migratedFromCaret: boolean; +} + +const SECTIONS: DependencySection[] = ['dependencies', 'devDependencies']; + +function findDeclared(manifest: PackageManifest, name: string): { section: DependencySection; raw: string } | null { + for (const section of SECTIONS) { + const raw = manifest[section]?.[name]; + if (raw !== undefined) return { section, raw }; + } + return null; +} + +/** + * Skew exists when the project's declared base version is ahead of the CLI's + * expectation in a way patch-floating can't explain: a higher major.minor for + * ranged deps, or any prerelease-aware greater-than for exact pins (so an + * alpha.20 project is never silently downgraded to an alpha.19 CLI's pin). + */ +function isSkewed(declared: ParsedSpecifier, expected: ParsedSpecifier): boolean { + if (declared.kind === 'unsupported' || expected.kind === 'unsupported') return false; + const d = declared.version; + const e = expected.version; + if (expected.kind === 'exact') return compareVersions(d, e) > 0; + if (d.major !== e.major) return d.major > e.major; + return d.minor > e.minor; +} + +export function computeSyncPlan(vended: PackageManifest, project: PackageManifest): SyncPlan { + const plan: SyncPlan = { skew: [], changes: [], restored: [], skipped: [], migratedFromCaret: false }; + + for (const section of SECTIONS) { + for (const [name, expectedRaw] of Object.entries(vended[section] ?? {})) { + const expected = parseSpecifier(expectedRaw); + const declared = findDeclared(project, name); + + if (!declared) { + // Managed dep deleted by the user — the vended CDK app can't build without it. + plan.restored.push({ name, section, to: expectedRaw }); + continue; + } + + if (declared.raw === expectedRaw) continue; + + const declaredSpec = parseSpecifier(declared.raw); + if (declaredSpec.kind === 'unsupported') { + // file:/git:/tag/range specifiers (incl. the bundled-tarball override) are + // deliberate local overrides — never rewrite, never skew-check. + plan.skipped.push({ + name, + raw: declared.raw, + reason: 'uses a non-semver specifier and was left unmanaged', + }); + continue; + } + + if (isSkewed(declaredSpec, expected)) { + plan.skew.push({ name, declared: declared.raw, expected: expectedRaw }); + continue; + } + + if (declaredSpec.kind === 'caret') plan.migratedFromCaret = true; + plan.changes.push({ name, section: declared.section, from: declared.raw, to: expectedRaw }); + } + } + + return plan; +} diff --git a/src/lib/dependency-management/semver.ts b/src/lib/dependency-management/semver.ts new file mode 100644 index 000000000..ebd7bc33c --- /dev/null +++ b/src/lib/dependency-management/semver.ts @@ -0,0 +1,96 @@ +/** + * Minimal semver parsing and comparison for managed-dependency syncing. + * + * Deliberately self-contained (no imports): this module must compare prerelease + * versions correctly (0.1.0-alpha.19 < 0.1.0-alpha.20 < 0.1.0), which the + * dependency-check parser in src/cli/external-requirements/versions.ts does not — + * it drops prerelease tags, which here would make an alpha downgrade look like a no-op. + */ + +export interface ParsedVersion { + major: number; + minor: number; + patch: number; + /** Dot-separated prerelease identifiers, e.g. ['alpha', 19]. Empty for release versions. */ + prerelease: (string | number)[]; +} + +export type SpecifierKind = 'exact' | 'tilde' | 'caret'; + +export type ParsedSpecifier = + | { kind: SpecifierKind; version: ParsedVersion; raw: string } + | { kind: 'unsupported'; raw: string }; + +const VERSION_RE = /^(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-.]+)?$/; + +export function parseVersion(version: string): ParsedVersion | null { + const match = VERSION_RE.exec(version.trim()); + if (!match) return null; + const prerelease = match[4] ? match[4].split('.').map(id => (/^\d+$/.test(id) ? Number(id) : id)) : []; + return { + major: Number(match[1]), + minor: Number(match[2]), + patch: Number(match[3]), + prerelease, + }; +} + +/** + * Compare two versions per the semver spec, including prerelease precedence: + * numeric identifiers compare numerically and rank below alphanumeric ones, + * and a prerelease version ranks below its release (1.0.0-alpha < 1.0.0). + */ +export function compareVersions(a: ParsedVersion, b: ParsedVersion): number { + if (a.major !== b.major) return a.major - b.major; + if (a.minor !== b.minor) return a.minor - b.minor; + if (a.patch !== b.patch) return a.patch - b.patch; + + if (a.prerelease.length === 0 && b.prerelease.length === 0) return 0; + if (a.prerelease.length === 0) return 1; + if (b.prerelease.length === 0) return -1; + + const len = Math.max(a.prerelease.length, b.prerelease.length); + for (let i = 0; i < len; i++) { + const idA = a.prerelease[i]; + const idB = b.prerelease[i]; + // A larger identifier set has higher precedence (1.0.0-alpha < 1.0.0-alpha.1) + if (idA === undefined) return -1; + if (idB === undefined) return 1; + if (idA === idB) continue; + if (typeof idA === 'number' && typeof idB === 'number') return idA - idB; + if (typeof idA === 'number') return -1; // numeric identifiers rank below alphanumeric + if (typeof idB === 'number') return 1; + return idA < idB ? -1 : 1; + } + return 0; +} + +/** + * Parse an npm dependency specifier into exact / tilde / caret + base version. + * Anything else (file:, git:, workspace:, URLs, wildcards, compound ranges, tags) + * is 'unsupported' — the sync skips those rather than guessing. + */ +export function parseSpecifier(raw: string): ParsedSpecifier { + const trimmed = raw.trim(); + let kind: SpecifierKind = 'exact'; + let versionPart = trimmed; + + if (trimmed.startsWith('~')) { + kind = 'tilde'; + versionPart = trimmed.slice(1); + } else if (trimmed.startsWith('^')) { + kind = 'caret'; + versionPart = trimmed.slice(1); + } else if (trimmed.startsWith('=')) { + versionPart = trimmed.slice(1); + } + + const version = parseVersion(versionPart); + if (!version) return { kind: 'unsupported', raw }; + return { kind, version, raw }; +} + +export function formatVersion(v: ParsedVersion): string { + const base = `${v.major}.${v.minor}.${v.patch}`; + return v.prerelease.length > 0 ? `${base}-${v.prerelease.join('.')}` : base; +} diff --git a/src/lib/dependency-management/types.ts b/src/lib/dependency-management/types.ts new file mode 100644 index 000000000..5fa771e86 --- /dev/null +++ b/src/lib/dependency-management/types.ts @@ -0,0 +1,54 @@ +export type DependencySection = 'dependencies' | 'devDependencies'; + +/** A package.json shape that preserves unknown fields verbatim. */ +export interface PackageManifest { + dependencies?: Record; + devDependencies?: Record; + [key: string]: unknown; +} + +export interface DependencyChange { + name: string; + section: DependencySection; + from: string; + to: string; +} + +export interface RestoredDependency { + name: string; + section: DependencySection; + to: string; +} + +export interface SkippedDependency { + name: string; + raw: string; + reason: string; +} + +export interface DependencySyncResult { + /** True when dependency management is disabled via global config — nothing was written. */ + optedOut: boolean; + /** True when the project predated pinning (caret ranges on managed deps were rewritten). */ + migrated: boolean; + /** True when node_modules + lockfile were deleted and reinstalled. */ + reinstalled: boolean; + /** True when a newer-than-CLI managed dep was found while opted out (warned instead of thrown). */ + skewWarning: boolean; + changes: DependencyChange[]; + restored: RestoredDependency[]; + skipped: SkippedDependency[]; + /** Ready-to-print warning lines (opted-out skew, skipped specifiers). */ + warnings: string[]; + /** Ready-to-print user notice (migration message and/or change summary), null if nothing to say. */ + notice: string | null; +} + +export interface SyncManagedDependenciesOptions { + /** Path to the CLI's vended CDK package.json — source of truth for managed names + versions. */ + vendedPackageJsonPath: string; + /** The user project's CDK directory (contains package.json, node_modules, lockfile). */ + projectDir: string; + /** From global config `disableDependencyManagement`. When true: no writes, skew becomes a warning. */ + disabled?: boolean; +} diff --git a/src/lib/errors/types.ts b/src/lib/errors/types.ts index 93345263d..6186ea39c 100644 --- a/src/lib/errors/types.ts +++ b/src/lib/errors/types.ts @@ -139,6 +139,27 @@ export class StaleCdkConstructError extends BaseError { } } +/** + * Error thrown during deploy preflight when the project's agentcore/cdk/package.json + * declares a managed dependency newer (minor/major, or prerelease-aware for exact pins) + * than this CLI release expects — i.e. the project was updated by a newer CLI. + */ +export class CliVersionTooOldError extends BaseError { + constructor(message: string, options?: BaseErrorOptions) { + super(message, { defaultSource: 'user', ...options }); + } +} + +/** + * Error thrown when the managed-dependency sync fails to rewrite package.json or + * reinstall the CDK project's dependencies (fs error, npm install failure). + */ +export class DependencySyncError extends BaseError { + constructor(message: string, options?: BaseErrorOptions) { + super(message, { defaultSource: 'client', ...options }); + } +} + /** * Error thrown when AWS credentials are not configured or invalid. * Supports both a short message (for interactive mode) and detailed message (for CLI mode). diff --git a/src/lib/schemas/io/global-config.ts b/src/lib/schemas/io/global-config.ts index 3046db8b4..a7b92657c 100644 --- a/src/lib/schemas/io/global-config.ts +++ b/src/lib/schemas/io/global-config.ts @@ -17,6 +17,7 @@ const GlobalConfigSchemaStrict = z installationId: z.string().uuid().optional(), uvDefaultIndex: z.string().optional(), uvIndex: z.string().optional(), + disableDependencyManagement: z.boolean().optional(), disableTransactionSearch: z.boolean().optional(), transactionSearchIndexPercentage: z.number().int().min(0).max(100).optional(), telemetry: z From 3eb1dcdf017197a07b68b6b697fbfc72ff39c0e3 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Fri, 17 Jul 2026 01:22:07 +0000 Subject: [PATCH 2/8] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20correct=20install=20command,=20non-destructive=20sy?= =?UTF-8?q?nc,=20preview/teardown=20safety,=20surfaced=20warnings,=20dev?= =?UTF-8?q?=20notices,=20failure=20telemetry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - messages.ts: drop the wrong @aws/agentcore-cli package name; install command is now threaded in from the CLI adapter (getDistroConfig().installCommand) with a standalone default, keeping src/lib free of src/cli imports - index.ts: never delete node_modules or the lockfile — npm reconciles the edited package.json incrementally; install also runs when node_modules is missing so a previously failed install self-heals on retry - skew error now names the skewed deps, uses the distro-aware install command, and points at the disableDependencyManagement opt-out for intentional manual bumps - mode: 'check' computes the plan/warnings/notice (future tense) without writing or installing; deploy --dry-run/--diff (CLI + TUI diffMode) run check-only, and teardown deploys downgrade skew to a warning so it can't block destroying a stack - dep-sync warnings now reach the terminal via postDeployWarnings (CLI) and a rendered warnings block (DeployScreen); dev flows print/render the notice + warnings too (runCliDeploy console output, useDevDeploy reads them from the deploy result instead of multiplexing onNotice, DevScreen renders them in their own block) - depSync hoisted out of the try in handleDeploy and carried on the failure result; command.tsx records dep_sync_* attrs regardless of success via a shared toDepSyncAttrs() also used by useDeployFlow - shared SYNC_CDK_DEPENDENCIES_STEP constant; sync skipped under AGENTCORE_SKIP_INSTALL --- src/cli/commands/deploy/actions.ts | 27 ++++-- src/cli/commands/deploy/command.tsx | 14 ++- src/cli/commands/deploy/progress.ts | 9 ++ src/cli/commands/deploy/types.ts | 5 +- src/cli/commands/deploy/utils.ts | 15 ++++ src/cli/operations/deploy/dependency-sync.ts | 54 +++++++++-- src/cli/operations/deploy/index.ts | 7 +- .../tui/hooks/__tests__/useDevDeploy.test.tsx | 63 ++++++++++--- src/cli/tui/hooks/useCdkPreflight.ts | 33 +++++-- src/cli/tui/hooks/useDevDeploy.ts | 34 ++++++- src/cli/tui/screens/deploy/DeployScreen.tsx | 12 +++ src/cli/tui/screens/deploy/useDeployFlow.ts | 20 ++--- src/cli/tui/screens/dev/DevScreen.tsx | 18 ++++ .../__tests__/sync.test.ts | 90 +++++++++++++++++-- src/lib/dependency-management/index.ts | 90 ++++++++++++++----- src/lib/dependency-management/messages.ts | 63 ++++++++++--- src/lib/dependency-management/types.ts | 24 ++++- 17 files changed, 478 insertions(+), 100 deletions(-) diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 3509109fc..61b438d40 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -1,4 +1,5 @@ import { ConfigIO, ResourceNotFoundError, SecureCredentials, ValidationError, toError } from '../../../lib'; +import type { DependencySyncResult } from '../../../lib/dependency-management'; import type { Result } from '../../../lib/result'; import type { AgentCoreMcpSpec, DeployedState } from '../../../schema'; import { applyTargetRegionToEnv } from '../../aws'; @@ -26,6 +27,7 @@ import { getErrorMessage } from '../../errors'; import { ExecLogger } from '../../logging'; import { MANAGED_MEMORY_DEPLOY_NOTICE, + SYNC_CDK_DEPENDENCIES_STEP, assertEnvFileExists, backfillContainerVpcIds, bootstrapEnvironment, @@ -169,6 +171,9 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { currentStepName = name; @@ -265,8 +270,15 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { const result = await executeDeploy(options).catch( (e): DeployResult => ({ success: false, error: e instanceof Error ? e : new Error(getErrorMessage(e)) }) ); - if (result.success && result.dependencySync) { - recorder.set({ - dep_sync_changed_count: result.dependencySync.changes.length + result.dependencySync.restored.length, - dep_sync_migrated: result.dependencySync.migrated, - dep_sync_opted_out: result.dependencySync.optedOut, - dep_sync_skew_warning: result.dependencySync.skewWarning, - dep_sync_reinstalled: result.dependencySync.reinstalled, - }); + // Record dep_sync attrs whenever the sync ran — including on failed deploys, where the + // failure result still carries the outcome (see handleDeploy's catch). + if (result.dependencySync) { + recorder.set(toDepSyncAttrs(result.dependencySync)); } if (!result.success) { return { success: false as const, error: result.error, deployResult: result }; diff --git a/src/cli/commands/deploy/progress.ts b/src/cli/commands/deploy/progress.ts index da7eb7b3c..d73b77038 100644 --- a/src/cli/commands/deploy/progress.ts +++ b/src/cli/commands/deploy/progress.ts @@ -68,6 +68,15 @@ export async function runCliDeploy(): Promise { }); cleanup(); + // Surface the managed-dependency sync outcome (#1540) — handleDeploy only writes it to the + // deploy log; this dev path passes no onNotice, so print it here. + if (result.dependencySync?.notice) { + console.log(`\n${result.dependencySync.notice}\n`); + } + for (const warning of result.dependencySync?.warnings ?? []) { + console.warn(`${ANSI.yellow}⚠ ${warning}${ANSI.reset}`); + } + if (result.success) { console.log('Deploy complete.'); if (result.logPath) { diff --git a/src/cli/commands/deploy/types.ts b/src/cli/commands/deploy/types.ts index ec066de4e..d2a20d7be 100644 --- a/src/cli/commands/deploy/types.ts +++ b/src/cli/commands/deploy/types.ts @@ -18,8 +18,11 @@ export type DeployResult = Result<{ nextSteps?: string[]; notes?: string[]; postDeployWarnings?: string[]; +}> & { + logPath?: string; + /** Sync outcome rides on BOTH branches so dep_sync_* telemetry survives a failed deploy. */ dependencySync?: DependencySyncResult; -}> & { logPath?: string }; +}; export type PreflightResult = Result<{ stackNames?: string[]; diff --git a/src/cli/commands/deploy/utils.ts b/src/cli/commands/deploy/utils.ts index d6362e114..ea95becb6 100644 --- a/src/cli/commands/deploy/utils.ts +++ b/src/cli/commands/deploy/utils.ts @@ -1,3 +1,4 @@ +import type { DependencySyncResult } from '../../../lib/dependency-management'; import type { AgentCoreProjectSpec } from '../../../schema'; import type { DeployMode } from '../../telemetry/schemas/common-shapes'; @@ -15,6 +16,20 @@ export const DEFAULT_DEPLOY_ATTRS = { deploy_mode: 'deploy' as DeployMode, }; +/** + * Map a managed-dependency sync outcome (#1540) to its dep_sync_* telemetry attrs. + * Single source for both the CLI command (command.tsx) and the TUI flow (useDeployFlow). + */ +export function toDepSyncAttrs(sync: DependencySyncResult) { + return { + dep_sync_changed_count: sync.changes.length + sync.restored.length, + dep_sync_migrated: sync.migrated, + dep_sync_opted_out: sync.optedOut, + dep_sync_skew_warning: sync.skewWarning, + dep_sync_reinstalled: sync.reinstalled, + }; +} + export function computeDeployAttrs(projectSpec: Partial, mode: DeployMode) { const gateways = projectSpec.agentCoreGateways ?? []; const policyEngines = projectSpec.policyEngines ?? []; diff --git a/src/cli/operations/deploy/dependency-sync.ts b/src/cli/operations/deploy/dependency-sync.ts index a54b6999a..b0715c7b0 100644 --- a/src/cli/operations/deploy/dependency-sync.ts +++ b/src/cli/operations/deploy/dependency-sync.ts @@ -2,23 +2,65 @@ import { syncManagedDependencies } from '../../../lib/dependency-management'; import type { DependencySyncResult } from '../../../lib/dependency-management'; import { readGlobalConfigSync } from '../../../lib/schemas/io/global-config'; import type { LocalCdkProject } from '../../cdk/local-cdk-project'; +import { getDistroConfig } from '../../constants'; import { getTemplatePath } from '../../templates/templateRoot'; +/** Step label for the managed-dependency sync, shared by the CLI and TUI deploy flows. */ +export const SYNC_CDK_DEPENDENCIES_STEP = 'Sync CDK dependencies'; + +export interface EnsureManagedDependenciesOptions { + /** + * Check-only run for preview modes (--dry-run / --diff): computes the plan, warnings, and a + * future-tense notice without writing package.json or running npm install. + */ + checkOnly?: boolean; + /** + * Teardown deploys: newer-than-CLI skew becomes a warning instead of CliVersionTooOldError, + * so skew never blocks destroying a cost-incurring stack. + */ + treatSkewAsWarning?: boolean; +} + +/** No-op result for when the sync is skipped entirely (AGENTCORE_SKIP_INSTALL). */ +const SKIPPED_SYNC_RESULT: DependencySyncResult = { + optedOut: false, + checkOnly: true, + migrated: false, + reinstalled: false, + skewWarning: false, + changes: [], + restored: [], + skipped: [], + warnings: [], + notice: null, +}; + /** * Deploy-preflight entry point for managed dependency pinning (#1540): resolves the - * CLI's vended CDK package.json (source of truth) and the global opt-out, then runs - * the sync against the project's agentcore/cdk directory. A future `agentcore build` - * command should call this same function. + * CLI's vended CDK package.json (source of truth), the global opt-out, and the + * distro-aware CLI install command, then runs the sync against the project's + * agentcore/cdk directory. A future `agentcore build` command should call this same + * function. Skipped entirely when AGENTCORE_SKIP_INSTALL is set (same escape hatch + * as create --no-install and the language setup steps). * - * Throws CliVersionTooOldError when the project was updated by a newer CLI, and - * DependencySyncError on rewrite/reinstall failure. + * Throws CliVersionTooOldError when the project was updated by a newer CLI (unless + * `treatSkewAsWarning` or opted out), and DependencySyncError on rewrite/install failure. */ -export async function ensureManagedDependencies(cdkProject: LocalCdkProject): Promise { +export async function ensureManagedDependencies( + cdkProject: LocalCdkProject, + options: EnsureManagedDependenciesOptions = {} +): Promise { + if (process.env.AGENTCORE_SKIP_INSTALL) { + return SKIPPED_SYNC_RESULT; + } const disabled = readGlobalConfigSync().disableDependencyManagement === true; return syncManagedDependencies({ vendedPackageJsonPath: getTemplatePath('cdk', 'package.json'), projectDir: cdkProject.projectDir, disabled, + mode: options.checkOnly ? 'check' : 'apply', + treatSkewAsWarning: options.treatSkewAsWarning, + installCommand: getDistroConfig().installCommand, }); } diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index 62b3e2701..271acd15b 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -69,7 +69,12 @@ export { ensureDefaultDeploymentTarget } from './ensure-target'; export { backfillContainerVpcIds, type BackfillVpcIdResult } from './backfill-vpc-id'; // Managed dependency pinning (#1540) — shared by CLI deploy + TUI preflight -export { ensureManagedDependencies, type DependencySyncResult } from './dependency-sync'; +export { + ensureManagedDependencies, + SYNC_CDK_DEPENDENCIES_STEP, + type DependencySyncResult, + type EnsureManagedDependenciesOptions, +} from './dependency-sync'; // Managed-memory heads-up (shared by the CLI command + TUI deploy flow + add harness) export { diff --git a/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx b/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx index d4130ccd0..b81e8d9eb 100644 --- a/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx +++ b/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx @@ -3,14 +3,19 @@ import { Text } from 'ink'; import { render } from 'ink-testing-library'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { mockHandleDeploy, mockReadProjectSpec, mockEnsureDefaultDeploymentTarget, mockCanSkipDeploy } = vi.hoisted( - () => ({ - mockHandleDeploy: vi.fn(), - mockReadProjectSpec: vi.fn(), - mockEnsureDefaultDeploymentTarget: vi.fn(), - mockCanSkipDeploy: vi.fn(), - }) -); +const { + mockHandleDeploy, + mockReadProjectSpec, + mockEnsureDefaultDeploymentTarget, + mockCanSkipDeploy, + TEST_MANAGED_MEMORY_NOTICE, +} = vi.hoisted(() => ({ + mockHandleDeploy: vi.fn(), + mockReadProjectSpec: vi.fn(), + mockEnsureDefaultDeploymentTarget: vi.fn(), + mockCanSkipDeploy: vi.fn(), + TEST_MANAGED_MEMORY_NOTICE: 'Managed memory: this harness provisions a dedicated AgentCore Memory resource', +})); vi.mock('../../../commands/deploy/actions.js', () => ({ handleDeploy: (...args: unknown[]) => mockHandleDeploy(...args), @@ -29,6 +34,7 @@ vi.mock('../../../../lib', async importActual => ({ vi.mock('../../../operations/deploy', () => ({ ensureDefaultDeploymentTarget: (...args: unknown[]) => mockEnsureDefaultDeploymentTarget(...args), + MANAGED_MEMORY_DEPLOY_NOTICE: TEST_MANAGED_MEMORY_NOTICE, })); vi.mock('../../../operations/deploy/change-detection', () => ({ @@ -36,11 +42,14 @@ vi.mock('../../../operations/deploy/change-detection', () => ({ })); function Harness({ skip }: { skip?: boolean }) { - const { steps, isComplete, error, managedMemoryNotice } = useDevDeploy({ skip }); + const { steps, isComplete, error, managedMemoryNotice, dependencySyncNotice, dependencySyncWarnings } = useDevDeploy({ + skip, + }); return ( steps:{steps.length} isComplete:{String(isComplete)} error:{error ?? 'null'} notice: - {managedMemoryNotice ?? 'null'} + {managedMemoryNotice ?? 'null'} depSyncNotice:{dependencySyncNotice ?? 'null'} depSyncWarnings: + {dependencySyncWarnings.length} ); } @@ -111,7 +120,7 @@ describe('useDevDeploy', () => { it('surfaces the managed-memory heads-up from the onNotice callback', async () => { mockHandleDeploy.mockImplementation((opts: { onNotice?: (message: string) => void }) => { - opts.onNotice?.('Managed memory: this harness automatically provisions a dedicated AgentCore Memory resource'); + opts.onNotice?.(TEST_MANAGED_MEMORY_NOTICE); return Promise.resolve({ success: true }); }); @@ -123,6 +132,38 @@ describe('useDevDeploy', () => { }); }); + it('does not let other onNotice messages clobber the managed-memory slot', async () => { + mockHandleDeploy.mockImplementation((opts: { onNotice?: (message: string) => void }) => { + opts.onNotice?.('Updated managed dependencies in agentcore/cdk/package.json:'); + return Promise.resolve({ success: true }); + }); + + const { lastFrame } = render(); + + await vi.waitFor(() => { + expect(lastFrame()).toContain('isComplete:true'); + }); + expect(lastFrame()).toContain('notice:null'); + }); + + it('reads the dependency sync notice and warnings from the deploy result', async () => { + mockHandleDeploy.mockResolvedValue({ + success: true, + dependencySync: { + notice: 'dep-sync-notice', + warnings: ['lodash (file:x) uses a non-semver specifier and was left unmanaged.'], + }, + }); + + const { lastFrame } = render(); + + await vi.waitFor(() => { + expect(lastFrame()).toContain('depSyncNotice:dep-sync-notice'); + expect(lastFrame()).toContain('depSyncWarnings:1'); + expect(lastFrame()).toContain('isComplete:true'); + }); + }); + it('leaves the managed-memory heads-up null when onNotice is not called', async () => { mockHandleDeploy.mockResolvedValue({ success: true }); diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index 6000dba61..b47c80950 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -10,6 +10,7 @@ import type { ExecLogger } from '../../logging'; import { type MissingCredential, type PreflightContext, + SYNC_CDK_DEPENDENCIES_STEP, bootstrapEnvironment, buildCdkProject, checkBootstrapNeeded, @@ -144,6 +145,12 @@ export interface PreflightOptions { isInteractive?: boolean; /** Skip identity provider check (for plan command which only synthesizes) */ skipIdentityCheck?: boolean; + /** + * Preview mode (diff): the managed-dependency sync runs check-only, computing the plan and a + * future-tense notice without writing package.json or running npm install. Previews must never + * mutate the working tree. + */ + dependencySyncCheckOnly?: boolean; } export interface PreflightResult { @@ -196,7 +203,7 @@ const STEP_BUILD = 3; const BASE_PREFLIGHT_STEPS: Step[] = [ { label: 'Validate project', status: 'pending' }, { label: 'Check dependencies', status: 'pending' }, - { label: 'Sync CDK dependencies', status: 'pending' }, + { label: SYNC_CDK_DEPENDENCIES_STEP, status: 'pending' }, { label: 'Build CDK project', status: 'pending' }, { label: 'Synthesize CloudFormation', status: 'pending' }, { label: 'Check stack status', status: 'pending' }, @@ -211,7 +218,7 @@ const IDENTITY_STEP: Step = { label: LABEL_API_KEY, status: 'pending' }; const BOOTSTRAP_STEP: Step = { label: 'Bootstrap AWS environment', status: 'pending' }; export function useCdkPreflight(options: PreflightOptions): PreflightResult { - const { logger, isInteractive = false, skipIdentityCheck = false } = options; + const { logger, isInteractive = false, skipIdentityCheck = false, dependencySyncCheckOnly = false } = options; // Create switchable ioHost - starts silent, can be flipped to verbose for deploy const switchableIoHost = useMemo(() => createSwitchableIoHost(), []); @@ -494,11 +501,16 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { // Step: Sync managed dependencies (#1540) — pin agentcore/cdk/package.json to the versions // this CLI was tested with, migrating pre-pinning (caret) projects. Runs before the build so - // the compile sees the reinstalled tree. + // the compile sees the reinstalled tree. Diff mode runs check-only (never mutates the + // working tree), and teardown deploys downgrade skew to a warning so it can't block + // destroying a cost-incurring stack. updateStep(STEP_SYNC_DEPS, { status: 'running' }); - logger.startStep('Sync CDK dependencies'); + logger.startStep(SYNC_CDK_DEPENDENCIES_STEP); try { - const syncResult = await ensureManagedDependencies(preflightContext.cdkProject); + const syncResult = await ensureManagedDependencies(preflightContext.cdkProject, { + checkOnly: dependencySyncCheckOnly, + treatSkewAsWarning: preflightContext.isTeardownDeploy, + }); for (const warning of syncResult.warnings) { logger.log(warning, 'warn'); } @@ -663,7 +675,16 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { return () => { process.off('unhandledRejection', handleUnhandledRejection); }; - }, [phase, logger, switchableIoHost, isInteractive, skipIdentityCheck, teardownConfirmed, restoreRegionEnv]); + }, [ + phase, + logger, + switchableIoHost, + isInteractive, + skipIdentityCheck, + dependencySyncCheckOnly, + teardownConfirmed, + restoreRegionEnv, + ]); // Handle identity-setup phase (after user provides credentials) useEffect(() => { diff --git a/src/cli/tui/hooks/useDevDeploy.ts b/src/cli/tui/hooks/useDevDeploy.ts index d6615f8ee..2fbaf15d0 100644 --- a/src/cli/tui/hooks/useDevDeploy.ts +++ b/src/cli/tui/hooks/useDevDeploy.ts @@ -2,7 +2,7 @@ import { ConfigIO } from '../../../lib'; import type { DeployMessage } from '../../cdk/toolkit-lib'; import { handleDeploy } from '../../commands/deploy/actions'; import { getErrorMessage } from '../../errors'; -import { ensureDefaultDeploymentTarget } from '../../operations/deploy'; +import { MANAGED_MEMORY_DEPLOY_NOTICE, ensureDefaultDeploymentTarget } from '../../operations/deploy'; import { canSkipDeploy } from '../../operations/deploy/change-detection'; import type { Step } from '../components/StepProgress'; import { useCallback, useEffect, useRef, useState } from 'react'; @@ -20,6 +20,10 @@ export interface UseDevDeployResult { logPath: string | undefined; /** Managed-memory heads-up surfaced by handleDeploy (null when not applicable) */ managedMemoryNotice: string | null; + /** Managed dependency sync summary (#1540) from the deploy result (null when nothing changed) */ + dependencySyncNotice: string | null; + /** Managed dependency sync warnings (#1540) from the deploy result */ + dependencySyncWarnings: string[]; } export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): UseDevDeployResult { @@ -29,6 +33,11 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): const [error, setError] = useState(); const [logPath, setLogPath] = useState(); const [managedMemoryNotice, setManagedMemoryNotice] = useState(null); + // Dep-sync gets its own slot: it's a multi-line summary read from the deploy RESULT, not the + // generic onNotice stream — multiplexing it through onNotice would let the managed-memory + // heads-up clobber it in the single "Note:" line. + const [dependencySyncNotice, setDependencySyncNotice] = useState(null); + const [dependencySyncWarnings, setDependencySyncWarnings] = useState([]); const hasStarted = useRef(false); const onProgress = useCallback((stepName: string, status: 'start' | 'success' | 'error') => { @@ -44,8 +53,13 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): setDeployMessages(prev => [...prev, msg]); }, []); + // onNotice is a generic stream (handleDeploy also emits the dep-sync notice through it); + // only the managed-memory heads-up belongs in the single "Note:" line — the dep-sync + // summary is read from the deploy result instead, so the two can't clobber each other. const onNotice = useCallback((message: string) => { - setManagedMemoryNotice(message); + if (message === MANAGED_MEMORY_DEPLOY_NOTICE) { + setManagedMemoryNotice(message); + } }, []); useEffect(() => { @@ -92,6 +106,11 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): setLogPath(result.logPath); } + if (result.dependencySync) { + setDependencySyncNotice(result.dependencySync.notice); + setDependencySyncWarnings(result.dependencySync.warnings); + } + if (!result.success) { setError(getErrorMessage(result.error)); } @@ -108,5 +127,14 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- skip is boolean, not nullable; || is the correct operator here const isComplete = skip || deployDone; - return { steps, deployMessages, isComplete, error, logPath, managedMemoryNotice }; + return { + steps, + deployMessages, + isComplete, + error, + logPath, + managedMemoryNotice, + dependencySyncNotice, + dependencySyncWarnings, + }; } diff --git a/src/cli/tui/screens/deploy/DeployScreen.tsx b/src/cli/tui/screens/deploy/DeployScreen.tsx index 3abe0e9aa..fb958084c 100644 --- a/src/cli/tui/screens/deploy/DeployScreen.tsx +++ b/src/cli/tui/screens/deploy/DeployScreen.tsx @@ -84,6 +84,7 @@ export function DeployScreen({ deployNotes, managedMemoryNotice, dependencySyncNotice, + dependencySyncWarnings, postDeployWarnings, postDeployHasError, isDiffLoading, @@ -352,6 +353,17 @@ export function DeployScreen({ )} + {/* Managed dependency sync warnings (#1540): downgraded skew, skipped specifiers. */} + {dependencySyncWarnings.length > 0 && ( + + {dependencySyncWarnings.map((warning, i) => ( + + ⚠ {warning} + + ))} + + )} + {/* Managed-memory heads-up: shown while the slow CFN apply runs (not gated on success). Styled as a plain dim "Note:" to match the transaction-search note convention below. */} {managedMemoryNotice && !isComplete && ( diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index c585bf009..3d0db9142 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -19,7 +19,7 @@ import { parsePolicyOutputs, parseRuntimeEndpointOutputs, } from '../../../cloudformation'; -import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs } from '../../../commands/deploy/utils.js'; +import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs, toDepSyncAttrs } from '../../../commands/deploy/utils.js'; import { toStackName } from '../../../commands/import/import-utils'; import { getErrorMessage, isChangesetInProgressError, isExpiredTokenError } from '../../../errors'; import { ExecLogger } from '../../../logging'; @@ -118,6 +118,8 @@ interface DeployFlowState { managedMemoryNotice: string | null; /** Managed dependency sync summary from preflight (#1540), null when nothing changed */ dependencySyncNotice: string | null; + /** Managed dependency sync warnings (downgraded skew, skipped specifiers) from preflight (#1540) */ + dependencySyncWarnings: string[]; /** Warnings from post-deploy steps (config bundles, AB tests) */ postDeployWarnings: string[]; /** True if any post-deploy sub-resource operation had errors */ @@ -146,14 +148,7 @@ interface DeployFlowState { /** Overlay dep_sync_* telemetry attrs from the preflight dependency sync (#1540), if it ran. */ function withDepSyncAttrs(attrs: T, sync: DependencySyncResult | null): T { if (!sync) return attrs; - return { - ...attrs, - dep_sync_changed_count: sync.changes.length + sync.restored.length, - dep_sync_migrated: sync.migrated, - dep_sync_opted_out: sync.optedOut, - dep_sync_skew_warning: sync.skewWarning, - dep_sync_reinstalled: sync.reinstalled, - }; + return { ...attrs, ...toDepSyncAttrs(sync) }; } export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState { @@ -163,8 +158,10 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState // Create logger once for the entire deploy flow const [logger] = useState(() => new ExecLogger({ command: 'deploy' })); - // Always call the hook (React rules), but we won't use it when preSynthesized is provided - const preflight = useCdkPreflight({ logger, isInteractive }); + // Always call the hook (React rules), but we won't use it when preSynthesized is provided. + // Diff mode is a preview: the managed-dependency sync runs check-only so the working tree + // is never mutated by `agentcore deploy --diff`. + const preflight = useCdkPreflight({ logger, isInteractive, dependencySyncCheckOnly: diffMode }); // Use pre-synthesized values when provided, otherwise use preflight values const cdkToolkitWrapper = preSynthesized?.cdkToolkitWrapper ?? preflight.cdkToolkitWrapper; @@ -1262,6 +1259,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState deployNotes, managedMemoryNotice, dependencySyncNotice: preflight.dependencySync?.notice ?? null, + dependencySyncWarnings: preflight.dependencySync?.warnings ?? [], postDeployWarnings, postDeployHasError, isDiffLoading, diff --git a/src/cli/tui/screens/dev/DevScreen.tsx b/src/cli/tui/screens/dev/DevScreen.tsx index bf51899cf..54081e2f9 100644 --- a/src/cli/tui/screens/dev/DevScreen.tsx +++ b/src/cli/tui/screens/dev/DevScreen.tsx @@ -264,6 +264,8 @@ export function DevScreen(props: DevScreenProps) { error: deployError, logPath: deployLogPath, managedMemoryNotice, + dependencySyncNotice, + dependencySyncWarnings, } = useDevDeploy({ skip: props.skipDeploy, ready: mode === 'deploying' }); const hasTransitionedFromDeployRef = useRef(false); @@ -548,6 +550,22 @@ export function DevScreen(props: DevScreenProps) { Note: {managedMemoryNotice} )} + {/* Managed dependency sync summary (#1540): its own block — the multi-line notice + doesn't fit the single "Note:" line above. */} + {dependencySyncNotice && ( + + {dependencySyncNotice} + + )} + {dependencySyncWarnings.length > 0 && ( + + {dependencySyncWarnings.map((warning, i) => ( + + ⚠ {warning} + + ))} + + )} {hasStartedCfn && ( diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts index 08e0ab4c6..5859dac06 100644 --- a/src/lib/dependency-management/__tests__/sync.test.ts +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -1,5 +1,6 @@ import { CliVersionTooOldError, DependencySyncError } from '../../errors/types'; import { syncManagedDependencies } from '../index'; +import type { SyncManagedDependenciesOptions } from '../index'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; @@ -30,13 +31,16 @@ describe('syncManagedDependencies', () => { writeFileSync(path.join(projectDir, 'package.json'), JSON.stringify(manifest, null, 2) + '\n'); }; const readProject = () => JSON.parse(readFileSync(path.join(projectDir, 'package.json'), 'utf-8')); - const run = (disabled = false) => - syncManagedDependencies({ vendedPackageJsonPath: vendedPath, projectDir, disabled }); + const run = (options: Partial = {}) => + syncManagedDependencies({ vendedPackageJsonPath: vendedPath, projectDir, ...options }); beforeEach(() => { tempDir = mkdtempSync(path.join(tmpdir(), 'dep-sync-test-')); projectDir = path.join(tempDir, 'agentcore', 'cdk'); mkdirSync(projectDir, { recursive: true }); + // Most tests exercise the manifest-diff path, not install self-healing — pre-create + // node_modules so a missing tree doesn't trigger the recovery install. + mkdirSync(path.join(projectDir, 'node_modules'), { recursive: true }); vendedPath = path.join(tempDir, 'vended-package.json'); writeFileSync(vendedPath, JSON.stringify(VENDED, null, 2)); mockRunSubprocessCapture.mockResolvedValue({ stdout: '', stderr: '', code: 0, signal: null }); @@ -56,8 +60,7 @@ describe('syncManagedDependencies', () => { expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); }); - it('migrates caret ranges, reinstalls, and reports', async () => { - mkdirSync(path.join(projectDir, 'node_modules'), { recursive: true }); + it('migrates caret ranges, reinstalls incrementally, and reports', async () => { writeFileSync(path.join(projectDir, 'package-lock.json'), '{}'); writeProject({ dependencies: { '@aws/agentcore-cdk': '^0.1.0-alpha.19', 'aws-cdk-lib': '^2.248.0', lodash: '^4.17.21' }, @@ -78,8 +81,21 @@ describe('syncManagedDependencies', () => { expect(written.dependencies['aws-cdk-lib']).toBe('~2.261.0'); expect(written.dependencies.lodash).toBe('^4.17.21'); - expect(existsSync(path.join(projectDir, 'node_modules'))).toBe(false); - expect(existsSync(path.join(projectDir, 'package-lock.json'))).toBe(false); + // The install is incremental: node_modules and the lockfile are never deleted, so + // user-added deps keep their resolved versions and installs stay warm. + expect(existsSync(path.join(projectDir, 'node_modules'))).toBe(true); + expect(existsSync(path.join(projectDir, 'package-lock.json'))).toBe(true); + expect(mockRunSubprocessCapture).toHaveBeenCalledWith('npm', ['install'], { cwd: projectDir }); + }); + + it('installs when node_modules is missing even if the manifest already matches (self-healing)', async () => { + rmSync(path.join(projectDir, 'node_modules'), { recursive: true, force: true }); + writeProject({ dependencies: { ...VENDED.dependencies }, devDependencies: { ...VENDED.devDependencies } }); + + const result = await run(); + + expect(result.changes).toEqual([]); + expect(result.reinstalled).toBe(true); expect(mockRunSubprocessCapture).toHaveBeenCalledWith('npm', ['install'], { cwd: projectDir }); }); @@ -104,13 +120,29 @@ describe('syncManagedDependencies', () => { await expect(run()).rejects.toThrow(/newer version of the AgentCore CLI/); }); + it('names the skewed dep, uses the provided install command, and mentions the opt-out in the skew error', async () => { + writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0' }, devDependencies: {} }); + const err = await run({ installCommand: 'npm install -g @aws/agentcore@preview' }).catch((e: unknown) => e); + expect(err).toBeInstanceOf(CliVersionTooOldError); + const message = (err as Error).message; + expect(message).toContain('aws-cdk-lib'); + expect(message).toContain('~2.300.0'); + expect(message).toContain('npm install -g @aws/agentcore@preview'); + expect(message).toContain('disableDependencyManagement'); + }); + + it('defaults the install command to the public package name', async () => { + writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0' }, devDependencies: {} }); + await expect(run()).rejects.toThrow(/npm install -g @aws\/agentcore@latest/); + }); + it('downgrades skew to a warning and touches nothing when disabled', async () => { const manifest = { dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0', old: '^1.0.0' }, devDependencies: { typescript: '~5.8.0' }, }; writeProject(manifest); - const result = await run(true); + const result = await run({ disabled: true }); expect(result.optedOut).toBe(true); expect(result.skewWarning).toBe(true); expect(result.warnings.some(w => w.includes('newer than this CLI was tested with'))).toBe(true); @@ -119,6 +151,45 @@ describe('syncManagedDependencies', () => { expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); }); + it('downgrades skew to a warning but still syncs when treatSkewAsWarning is set', async () => { + writeProject({ + dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0', '@aws/agentcore-cdk': '^0.1.0-alpha.19' }, + devDependencies: { typescript: '~5.9.3' }, + }); + const result = await run({ treatSkewAsWarning: true }); + expect(result.skewWarning).toBe(true); + expect(result.warnings.some(w => w.includes('newer than this CLI was tested with'))).toBe(true); + // The skewed dep is left alone; the other managed dep still syncs. + expect(readProject().dependencies['aws-cdk-lib']).toBe('~2.300.0'); + expect(readProject().dependencies['@aws/agentcore-cdk']).toBe('0.1.0-alpha.45'); + }); + + it('check mode computes the plan and a future-tense notice without writing or installing', async () => { + const manifest = { + dependencies: { '@aws/agentcore-cdk': '^0.1.0-alpha.19', 'aws-cdk-lib': '^2.248.0' }, + devDependencies: { typescript: '~5.9.3' }, + }; + writeProject(manifest); + const result = await run({ mode: 'check' }); + expect(result.checkOnly).toBe(true); + expect(result.changes).toHaveLength(2); + expect(result.reinstalled).toBe(false); + expect(result.notice).toContain('will be updated on the next deploy'); + expect(result.notice).not.toContain("We've updated"); + expect(readProject()).toEqual(manifest); + expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); + }); + + it('check mode reports skew as a warning instead of throwing', async () => { + const manifest = { dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0' }, devDependencies: {} }; + writeProject(manifest); + const result = await run({ mode: 'check' }); + expect(result.skewWarning).toBe(true); + expect(result.warnings.some(w => w.includes('newer than this CLI was tested with'))).toBe(true); + expect(readProject()).toEqual(manifest); + expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); + }); + it('restores a deleted managed dependency', async () => { writeProject({ dependencies: { '@aws/agentcore-cdk': '0.1.0-alpha.45' }, @@ -141,10 +212,13 @@ describe('syncManagedDependencies', () => { expect(readProject().dependencies['@aws/agentcore-cdk']).toBe('file:bundled-agentcore-cdk.tgz'); }); - it('wraps npm install failure in DependencySyncError', async () => { + it('wraps npm install failure in DependencySyncError and leaves the rewritten manifest for retry', async () => { mockRunSubprocessCapture.mockResolvedValue({ stdout: '', stderr: 'E404', code: 1, signal: null }); writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.250.0' }, devDependencies: {} }); await expect(run()).rejects.toThrow(DependencySyncError); + // package.json was already rewritten; the retry sees a matching manifest but recovers + // via the missing-node_modules install path (exercised in the self-healing test above). + expect(readProject().dependencies['aws-cdk-lib']).toBe('~2.261.0'); }); it('wraps unreadable project package.json in DependencySyncError', async () => { diff --git a/src/lib/dependency-management/index.ts b/src/lib/dependency-management/index.ts index 51f4e7ad6..4df8315fc 100644 --- a/src/lib/dependency-management/index.ts +++ b/src/lib/dependency-management/index.ts @@ -1,9 +1,16 @@ import { CliVersionTooOldError, DependencySyncError } from '../errors/types'; import { runSubprocessCapture } from '../utils/subprocess'; -import { CLI_UPGRADE_ERROR_MESSAGE, formatSkewWarning, formatSkippedWarning, formatSyncNotice } from './messages'; +import { + DEFAULT_CLI_INSTALL_COMMAND, + formatCliUpgradeError, + formatSkewWarning, + formatSkippedWarning, + formatSyncNotice, +} from './messages'; import { computeSyncPlan } from './policy'; import type { DependencySyncResult, PackageManifest, SyncManagedDependenciesOptions } from './types'; -import { readFile, rm, writeFile } from 'node:fs/promises'; +import { existsSync } from 'node:fs'; +import { readFile, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; export type { @@ -37,15 +44,29 @@ async function readManifest(path: string, what: string): Promise { - const { vendedPackageJsonPath, projectDir, disabled = false } = options; + const { + vendedPackageJsonPath, + projectDir, + disabled = false, + mode = 'apply', + treatSkewAsWarning = false, + installCommand = DEFAULT_CLI_INSTALL_COMMAND, + } = options; + const checkOnly = mode === 'check'; const projectPackageJsonPath = join(projectDir, 'package.json'); const vended = await readManifest(vendedPackageJsonPath, 'the vended CDK package.json'); @@ -55,6 +76,7 @@ export async function syncManagedDependencies(options: SyncManagedDependenciesOp const result: DependencySyncResult = { optedOut: disabled, + checkOnly, migrated: plan.migratedFromCaret, reinstalled: false, skewWarning: false, @@ -66,32 +88,55 @@ export async function syncManagedDependencies(options: SyncManagedDependenciesOp }; if (plan.skew.length > 0) { - if (!disabled) { - throw new CliVersionTooOldError(CLI_UPGRADE_ERROR_MESSAGE); + if (!disabled && !treatSkewAsWarning && !checkOnly) { + throw new CliVersionTooOldError(formatCliUpgradeError(plan.skew, installCommand)); } result.skewWarning = true; - result.warnings.push(formatSkewWarning(plan.skew)); + result.warnings.push(formatSkewWarning(plan.skew, installCommand)); } - if (disabled || (plan.changes.length === 0 && plan.restored.length === 0)) { + if (disabled) { return result; } - // Mutate the parsed manifest in place so user key order and unknown fields survive. - for (const change of plan.changes) { - project[change.section]![change.name] = change.to; + const hasPlannedChanges = plan.changes.length > 0 || plan.restored.length > 0; + + if (checkOnly) { + // Report what WOULD change without touching the working tree (future-tense wording). + result.notice = formatSyncNotice({ + migrated: result.migrated, + changes: result.changes, + restored: result.restored, + reinstalled: false, + applied: false, + }); + return result; } - for (const restoredDep of plan.restored) { - const section = (project[restoredDep.section] ??= {}); - section[restoredDep.name] = restoredDep.to; + + if (hasPlannedChanges) { + // Mutate the parsed manifest in place so user key order and unknown fields survive. + for (const change of plan.changes) { + project[change.section]![change.name] = change.to; + } + for (const restoredDep of plan.restored) { + const section = (project[restoredDep.section] ??= {}); + section[restoredDep.name] = restoredDep.to; + } + + try { + await writeFile(projectPackageJsonPath, JSON.stringify(project, null, 2) + '\n', 'utf-8'); + } catch (err) { + throw new DependencySyncError(`Failed to update the CDK project's dependencies: ${String(err)}`, { cause: err }); + } } - try { - await writeFile(projectPackageJsonPath, JSON.stringify(project, null, 2) + '\n', 'utf-8'); - await rm(join(projectDir, 'node_modules'), { recursive: true, force: true }); - await rm(join(projectDir, 'package-lock.json'), { force: true }); - } catch (err) { - throw new DependencySyncError(`Failed to update the CDK project's dependencies: ${String(err)}`, { cause: err }); + // Install when the manifest changed OR node_modules is missing (a previous failed install — + // package.json already matched on retry — self-heals here). npm >=7 reconciles the edited + // package.json against the existing lockfile incrementally, so we never delete node_modules + // or the lockfile: user-added deps keep their resolved versions and installs stay warm. + const needsInstall = hasPlannedChanges || !existsSync(join(projectDir, 'node_modules')); + if (!needsInstall) { + return result; } const install = await runSubprocessCapture('npm', ['install'], { cwd: projectDir }); @@ -107,6 +152,7 @@ export async function syncManagedDependencies(options: SyncManagedDependenciesOp changes: result.changes, restored: result.restored, reinstalled: result.reinstalled, + applied: true, }); return result; } diff --git a/src/lib/dependency-management/messages.ts b/src/lib/dependency-management/messages.ts index f72c1cc11..c72a18d3e 100644 --- a/src/lib/dependency-management/messages.ts +++ b/src/lib/dependency-management/messages.ts @@ -6,9 +6,12 @@ import type { DependencyChange, RestoredDependency, SkippedDependency } from './ * headless and TUI deploy paths (and any future CLI) can't drift apart. */ -export const CLI_UPGRADE_ERROR_MESSAGE = - 'This project requires a newer version of the AgentCore CLI. ' + - 'Run `npm install -g @aws/agentcore-cli@latest` and retry.'; +/** + * Fallback CLI install command when the caller doesn't thread one in. The CLI layer + * passes `getDistroConfig().installCommand` (dist-tag/registry aware); this default + * keeps the lib module usable standalone. + */ +export const DEFAULT_CLI_INSTALL_COMMAND = 'npm install -g @aws/agentcore@latest'; export const OPT_OUT_HINT = 'To manage these versions yourself: agentcore config disableDependencyManagement true'; @@ -17,6 +20,33 @@ const MIGRATION_PREAMBLE = "We've updated agentcore/cdk/package.json so the CLI keeps these dependencies\n" + 'on versions it has been tested with (patch updates still apply automatically):'; +// Check-mode variant: nothing was written, so the wording must promise rather than claim. +const MIGRATION_PREAMBLE_PENDING = + 'Your project was created before the AgentCore CLI managed dependency versions.\n' + + 'agentcore/cdk/package.json will be updated on the next deploy so the CLI keeps these\n' + + 'dependencies on versions it has been tested with (patch updates still apply automatically):'; + +function formatSkewList(skew: SkewFinding[]): string { + return skew.map(s => `${s.name} (${s.declared}, CLI expects ${s.expected})`).join(', '); +} + +/** + * Error text for newer-than-CLI skew. Names the skewed dependencies, gives the + * distro-correct upgrade command, and — because the skew may be a deliberate manual + * bump rather than a newer CLI — points at the opt-out as the alternative. + */ +export function formatCliUpgradeError(skew: SkewFinding[], installCommand: string): string { + const plural = skew.length > 1; + return ( + `This project requires a newer version of the AgentCore CLI: ${formatSkewList(skew)} ` + + `${plural ? 'are' : 'is'} newer than this CLI was tested with. ` + + `Run \`${installCommand}\` and retry.\n` + + `If you intentionally updated ${plural ? 'these dependencies' : 'this dependency'} yourself, ` + + 'you can disable managed dependency versions instead:\n' + + OPT_OUT_HINT + ); +} + function formatChangeTable(changes: DependencyChange[], restored: RestoredDependency[]): string { const rows: { name: string; from: string; to: string }[] = [ ...changes.map(c => ({ name: c.name, from: c.from, to: c.to })), @@ -32,34 +62,43 @@ export function formatSyncNotice(options: { changes: DependencyChange[]; restored: RestoredDependency[]; reinstalled: boolean; + /** False in check mode — nothing was written, so speak in the future tense. */ + applied: boolean; }): string | null { - const { migrated, changes, restored, reinstalled } = options; + const { migrated, changes, restored, reinstalled, applied } = options; if (changes.length === 0 && restored.length === 0) return null; const lines: string[] = []; if (migrated) { - lines.push(MIGRATION_PREAMBLE); + lines.push(applied ? MIGRATION_PREAMBLE : MIGRATION_PREAMBLE_PENDING); } else { - lines.push('Updated managed dependencies in agentcore/cdk/package.json:'); + lines.push( + applied + ? 'Updated managed dependencies in agentcore/cdk/package.json:' + : 'Managed dependencies in agentcore/cdk/package.json will be updated on the next deploy:' + ); } lines.push(''); lines.push(formatChangeTable(changes, restored)); lines.push(''); if (reinstalled) { - lines.push('Reinstalled agentcore/cdk dependencies.'); + lines.push('Ran npm install in agentcore/cdk to apply the updates.'); } - lines.push('Dependencies you added yourself were not changed.'); + lines.push( + applied + ? 'Dependencies you added yourself were not changed.' + : 'Dependencies you added yourself will not be changed.' + ); if (migrated) { lines.push(OPT_OUT_HINT); } return lines.join('\n'); } -export function formatSkewWarning(skew: SkewFinding[]): string { - const deps = skew.map(s => `${s.name} (${s.declared}, CLI expects ${s.expected})`).join(', '); +export function formatSkewWarning(skew: SkewFinding[], installCommand: string): string { return ( - `${deps} ${skew.length === 1 ? 'is' : 'are'} newer than this CLI was tested with; ` + - 'deploy may fail — upgrade the CLI with `npm install -g @aws/agentcore-cli@latest`.' + `${formatSkewList(skew)} ${skew.length === 1 ? 'is' : 'are'} newer than this CLI was tested with; ` + + `deploy may fail — upgrade the CLI with \`${installCommand}\`. ${OPT_OUT_HINT}` ); } diff --git a/src/lib/dependency-management/types.ts b/src/lib/dependency-management/types.ts index 5fa771e86..fa4dacb27 100644 --- a/src/lib/dependency-management/types.ts +++ b/src/lib/dependency-management/types.ts @@ -29,16 +29,18 @@ export interface SkippedDependency { export interface DependencySyncResult { /** True when dependency management is disabled via global config — nothing was written. */ optedOut: boolean; + /** True when this was a check-only run (`mode: 'check'`) — plan computed, nothing written or installed. */ + checkOnly: boolean; /** True when the project predated pinning (caret ranges on managed deps were rewritten). */ migrated: boolean; - /** True when node_modules + lockfile were deleted and reinstalled. */ + /** True when `npm install` was run to reconcile the installed tree (manifest changed, or node_modules was missing). */ reinstalled: boolean; - /** True when a newer-than-CLI managed dep was found while opted out (warned instead of thrown). */ + /** True when a newer-than-CLI managed dep was found but downgraded to a warning (opted out or skew-as-warning). */ skewWarning: boolean; changes: DependencyChange[]; restored: RestoredDependency[]; skipped: SkippedDependency[]; - /** Ready-to-print warning lines (opted-out skew, skipped specifiers). */ + /** Ready-to-print warning lines (downgraded skew, skipped specifiers). */ warnings: string[]; /** Ready-to-print user notice (migration message and/or change summary), null if nothing to say. */ notice: string | null; @@ -51,4 +53,20 @@ export interface SyncManagedDependenciesOptions { projectDir: string; /** From global config `disableDependencyManagement`. When true: no writes, skew becomes a warning. */ disabled?: boolean; + /** + * 'apply' (default) rewrites package.json and installs; 'check' only computes the plan and + * user-facing wording — no writes, no install. Used by preview modes (--dry-run / --diff), + * which must never mutate the working tree. + */ + mode?: 'apply' | 'check'; + /** + * Downgrade newer-than-CLI skew from CliVersionTooOldError to a warning even when managed. + * Used by teardown deploys, where skew must not block destroying a cost-incurring stack. + */ + treatSkewAsWarning?: boolean; + /** + * CLI upgrade command used in skew errors/warnings. The CLI layer passes the distro-aware + * `getDistroConfig().installCommand`; defaults keep the lib module usable standalone. + */ + installCommand?: string; } From 70d3163e8fb3bbdbf256c39398874f4c87e8bf27 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Fri, 17 Jul 2026 01:46:20 +0000 Subject: [PATCH 3/8] fix: keep dependency-sync warnings out of postDeployWarnings so informational warnings don't exit 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundled-tarball override (file:bundled-agentcore-cdk.tgz) always yields a 'left unmanaged' dep-sync warning in e2e/dev builds. Merging depSync.warnings into postDeployWarnings made every such deploy exit 2, failing e2e deploy assertions. Dep-sync warnings are informational: print them from printDeployResult (headless CLI) instead — the TUI and dev paths already render result.dependencySync.warnings separately. --- src/cli/commands/deploy/actions.ts | 10 +++++++--- src/cli/commands/deploy/command.tsx | 9 +++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 61b438d40..65f8efb4b 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -1001,9 +1001,13 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0) { + for (const warning of result.dependencySync.warnings) { + console.warn(`⚠ ${warning}`); + } + } + if (options.diff) { console.log(`\n✓ Diff complete for '${result.targetName}' (stack: ${result.stackName})`); } else if (options.plan) { From e0a2766e192ef34aa7258de9249b2b9c03fd59cf Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Fri, 17 Jul 2026 03:44:47 +0000 Subject: [PATCH 4/8] fix: carry depSync on failure returns, restore manifest on install failure, persist dev notice, print warnings on failed deploys, never block teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review fixes for managed dependency pinning (#1540): - handleDeploy: every explicit failure return after the sync step now carries dependencySync, so dep_sync_* telemetry and the rewrite notice survive deploys that fail at a later step (not just the catch path). - syncManagedDependencies: on npm install failure, restore the pre-rewrite package.json before throwing DependencySyncError — otherwise a retry sees a matching manifest plus a stale node_modules, skips the install, and deploys against the old installed tree (the exact skew the feature prevents). Tests pin restore + successful retry. - Dev TUI: the dep-sync notice/warnings arrived in the same render batch that transitioned out of 'deploying', so they were visible for at most one frame. They now persist into the post-deploy modes (harness chooser / InvokeScreen) and print to the normal buffer on the browser-launch path. - Headless CLI: the non-JSON failure branch now prints dependencySync.notice/warnings to stderr, so a failed deploy still explains the package.json rewrite and any downgraded skew. - Teardown: DependencySyncError (write/install failure) at both sync call sites is downgraded to a warning via teardownSyncFailureResult and the teardown proceeds — extending the treatSkewAsWarning invariant that nothing about pinning may block destroying a cost-incurring stack. --- src/cli/commands/deploy/actions.ts | 52 ++++++++++++---- src/cli/commands/deploy/command.tsx | 9 +++ src/cli/operations/deploy/dependency-sync.ts | 14 +++++ src/cli/operations/deploy/index.ts | 1 + src/cli/tui/hooks/useCdkPreflight.ts | 29 ++++++--- src/cli/tui/screens/dev/DevScreen.tsx | 60 +++++++++++++------ .../__tests__/sync.test.ts | 23 ++++++- src/lib/dependency-management/index.ts | 38 ++++++++---- 8 files changed, 175 insertions(+), 51 deletions(-) diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 65f8efb4b..cdfa067a6 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -1,4 +1,11 @@ -import { ConfigIO, ResourceNotFoundError, SecureCredentials, ValidationError, toError } from '../../../lib'; +import { + ConfigIO, + DependencySyncError, + ResourceNotFoundError, + SecureCredentials, + ValidationError, + toError, +} from '../../../lib'; import type { DependencySyncResult } from '../../../lib/dependency-management'; import type { Result } from '../../../lib/result'; import type { AgentCoreMcpSpec, DeployedState } from '../../../schema'; @@ -45,6 +52,7 @@ import { setupOAuth2Providers, setupTransactionSearch, synthesizeCdk, + teardownSyncFailureResult, validateProject, } from '../../operations/deploy'; import { computeProjectDeployHash } from '../../operations/deploy/change-detection'; @@ -171,8 +179,9 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { @@ -271,14 +280,25 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { if (options.json) { console.log(JSON.stringify(serializeResult(deployResult))); } else { + // Dependency sync outcome (#1540) still matters on a failed deploy: the notice explains + // the package.json rewrite that DID happen, and a downgraded-skew warning may explain the + // failure itself. printDeployResult only runs on success, so print them here. + if (deployResult.dependencySync?.notice) { + console.error(`${deployResult.dependencySync.notice}\n`); + } + for (const warning of deployResult.dependencySync?.warnings ?? []) { + console.error(`⚠ ${warning}`); + } console.error(deployResult.error.message); if (deployResult.logPath) { console.error(`Log: ${deployResult.logPath}`); diff --git a/src/cli/operations/deploy/dependency-sync.ts b/src/cli/operations/deploy/dependency-sync.ts index b0715c7b0..e4c4df07c 100644 --- a/src/cli/operations/deploy/dependency-sync.ts +++ b/src/cli/operations/deploy/dependency-sync.ts @@ -35,6 +35,20 @@ const SKIPPED_SYNC_RESULT: DependencySyncResult = { notice: null, }; +/** + * Warning-only result for a DependencySyncError caught on a teardown deploy. Teardown must + * never be blocked by pinning: skew is already downgraded via `treatSkewAsWarning`, and this + * extends the same invariant to write/install failures (broken npm env, registry outage) — + * the failure is surfaced through the normal warnings channel and the teardown proceeds. If + * node_modules is genuinely unusable, the build step fails on its own. + */ +export function teardownSyncFailureResult(err: Error): DependencySyncResult { + return { + ...SKIPPED_SYNC_RESULT, + warnings: [`Dependency sync failed (continuing with teardown): ${err.message}`], + }; +} + /** * Deploy-preflight entry point for managed dependency pinning (#1540): resolves the * CLI's vended CDK package.json (source of truth), the global opt-out, and the diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index 271acd15b..3be5ba91d 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -71,6 +71,7 @@ export { backfillContainerVpcIds, type BackfillVpcIdResult } from './backfill-vp // Managed dependency pinning (#1540) — shared by CLI deploy + TUI preflight export { ensureManagedDependencies, + teardownSyncFailureResult, SYNC_CDK_DEPENDENCIES_STEP, type DependencySyncResult, type EnsureManagedDependenciesOptions, diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index b47c80950..c3469a3c3 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -1,6 +1,6 @@ import { ConfigIO, SecureCredentials, toError } from '../../../lib'; import type { DependencySyncResult } from '../../../lib/dependency-management'; -import { AwsCredentialsError, UserCancellationError } from '../../../lib/errors/types'; +import { AwsCredentialsError, DependencySyncError, UserCancellationError } from '../../../lib/errors/types'; import type { DeployedState } from '../../../schema'; import { applyTargetRegionToEnv } from '../../aws'; import { validateAwsCredentials } from '../../aws/account'; @@ -24,6 +24,7 @@ import { setupApiKeyProviders, setupOAuth2Providers, synthesizeCdk, + teardownSyncFailureResult, validateProject, } from '../../operations/deploy'; import { @@ -502,8 +503,9 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { // Step: Sync managed dependencies (#1540) — pin agentcore/cdk/package.json to the versions // this CLI was tested with, migrating pre-pinning (caret) projects. Runs before the build so // the compile sees the reinstalled tree. Diff mode runs check-only (never mutates the - // working tree), and teardown deploys downgrade skew to a warning so it can't block - // destroying a cost-incurring stack. + // working tree), and teardown deploys downgrade every sync failure to a warning: skew via + // treatSkewAsWarning, write/install failures (DependencySyncError) via the catch below. + // Nothing about pinning may block destroying a cost-incurring stack. updateStep(STEP_SYNC_DEPS, { status: 'running' }); logger.startStep(SYNC_CDK_DEPENDENCIES_STEP); try { @@ -521,11 +523,22 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { logger.endStep('success'); updateStep(STEP_SYNC_DEPS, { status: 'success' }); } catch (err) { - const errorMsg = formatError(err); - logger.endStep('error', errorMsg); - updateStep(STEP_SYNC_DEPS, { status: 'error', error: getErrorMessage(err) }); - failPreflight(err); - return; + if (preflightContext.isTeardownDeploy && err instanceof DependencySyncError) { + // Surface through the same warning channels as a downgraded skew: the deploy-flow + // screens render dependencySync.warnings, and the step shows the warning inline. + const downgraded = teardownSyncFailureResult(err); + const warning = downgraded.warnings[0]!; + logger.log(warning, 'warn'); + setDependencySync(downgraded); + logger.endStep('success'); + updateStep(STEP_SYNC_DEPS, { status: 'warn', warn: warning }); + } else { + const errorMsg = formatError(err); + logger.endStep('error', errorMsg); + updateStep(STEP_SYNC_DEPS, { status: 'error', error: getErrorMessage(err) }); + failPreflight(err); + return; + } } // Step: Build CDK project diff --git a/src/cli/tui/screens/dev/DevScreen.tsx b/src/cli/tui/screens/dev/DevScreen.tsx index 54081e2f9..f2785b38e 100644 --- a/src/cli/tui/screens/dev/DevScreen.tsx +++ b/src/cli/tui/screens/dev/DevScreen.tsx @@ -275,6 +275,15 @@ export function DevScreen(props: DevScreenProps) { queueMicrotask(() => { if (onLaunchBrowser) { onLaunchBrowser({ harnessName: selectedHarness }); + // onLaunchBrowser exits the alt screen and unmounts synchronously, so anything rendered + // in 'deploying' mode is gone. Print the one-shot dep-sync outcome (#1540) — e.g. the + // "we rewrote your package.json" migration explanation — to the normal buffer instead. + if (dependencySyncNotice) { + console.log(`\n${dependencySyncNotice}\n`); + } + for (const warning of dependencySyncWarnings) { + console.warn(`⚠ ${warning}`); + } } else if (selectedHarness) { setMode('harness'); } else { @@ -519,6 +528,24 @@ export function DevScreen(props: DevScreenProps) { return null; } + // Managed dependency sync outcome (#1540): its own block — the multi-line notice doesn't fit + // the single "Note:" line in the deploying view. Rendered during the deploy AND in the + // post-deploy modes (harness / select-agent): the deploy completes and the mode transitions in + // the same render batch, so a block gated on mode === 'deploying' would be visible for at most + // one frame. The dev TUI runs in the alt screen, so persisting it in the UI is the only way + // the one-time "we rewrote your package.json" explanation is actually readable. + const dependencySyncSummary = + dependencySyncNotice || dependencySyncWarnings.length > 0 ? ( + + {dependencySyncNotice && {dependencySyncNotice}} + {dependencySyncWarnings.map((warning, i) => ( + + ⚠ {warning} + + ))} + + ) : null; + // Show error screen if no agents are defined if (noAgentsError) { return ( @@ -550,22 +577,7 @@ export function DevScreen(props: DevScreenProps) { Note: {managedMemoryNotice} )} - {/* Managed dependency sync summary (#1540): its own block — the multi-line notice - doesn't fit the single "Note:" line above. */} - {dependencySyncNotice && ( - - {dependencySyncNotice} - - )} - {dependencySyncWarnings.length > 0 && ( - - {dependencySyncWarnings.map((warning, i) => ( - - ⚠ {warning} - - ))} - - )} + {dependencySyncSummary && {dependencySyncSummary}} {hasStartedCfn && ( @@ -582,9 +594,19 @@ export function DevScreen(props: DevScreenProps) { ); } - // If harness mode, render the InvokeScreen with the pre-selected harness + // If harness mode, render the InvokeScreen with the pre-selected harness. The dep-sync + // summary stays visible above it — this mode is entered right after the deploy completes. if (mode === 'harness') { - return ; + return ( + + {dependencySyncSummary && ( + + {dependencySyncSummary} + + )} + + + ); } const statusColor = { starting: 'yellow', running: 'green', error: 'red', stopped: 'gray' }[status]; @@ -634,6 +656,8 @@ export function DevScreen(props: DevScreenProps) { return ( + {/* Post-deploy chooser: keep the dep-sync summary visible (see dependencySyncSummary). */} + {dependencySyncSummary && {dependencySyncSummary}} 0 ? 'Select Target' : 'Select Agent'} fullWidth> diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts index 5859dac06..f7e9aee37 100644 --- a/src/lib/dependency-management/__tests__/sync.test.ts +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -212,13 +212,30 @@ describe('syncManagedDependencies', () => { expect(readProject().dependencies['@aws/agentcore-cdk']).toBe('file:bundled-agentcore-cdk.tgz'); }); - it('wraps npm install failure in DependencySyncError and leaves the rewritten manifest for retry', async () => { + it('wraps npm install failure in DependencySyncError and restores the original manifest', async () => { mockRunSubprocessCapture.mockResolvedValue({ stdout: '', stderr: 'E404', code: 1, signal: null }); + const original = { dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.250.0' }, devDependencies: {} }; + writeProject(original); + await expect(run()).rejects.toThrow(DependencySyncError); + // The rewrite is rolled back on install failure. Otherwise a retry would see a manifest that + // already matches the pins plus a still-present node_modules, skip the install, and deploy + // against the stale installed tree — the exact skew this sync exists to prevent. + expect(readProject()).toEqual(original); + }); + + it('recomputes the same plan and re-attempts the install on retry after an install failure', async () => { + mockRunSubprocessCapture.mockResolvedValueOnce({ stdout: '', stderr: 'E404', code: 1, signal: null }); writeProject({ dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.250.0' }, devDependencies: {} }); await expect(run()).rejects.toThrow(DependencySyncError); - // package.json was already rewritten; the retry sees a matching manifest but recovers - // via the missing-node_modules install path (exercised in the self-healing test above). + + // Second run: the restored manifest yields the same plan, and this time npm succeeds. + const result = await run(); + expect(result.changes).toEqual([ + { name: 'aws-cdk-lib', section: 'dependencies', from: '~2.250.0', to: '~2.261.0' }, + ]); + expect(result.reinstalled).toBe(true); expect(readProject().dependencies['aws-cdk-lib']).toBe('~2.261.0'); + expect(mockRunSubprocessCapture).toHaveBeenCalledTimes(2); }); it('wraps unreadable project package.json in DependencySyncError', async () => { diff --git a/src/lib/dependency-management/index.ts b/src/lib/dependency-management/index.ts index 4df8315fc..3c7cc8d2b 100644 --- a/src/lib/dependency-management/index.ts +++ b/src/lib/dependency-management/index.ts @@ -21,7 +21,7 @@ export type { SyncManagedDependenciesOptions, } from './types'; -async function readManifest(path: string, what: string): Promise { +async function readManifest(path: string, what: string): Promise<{ manifest: PackageManifest; raw: string }> { let raw: string; try { raw = await readFile(path, 'utf-8'); @@ -29,7 +29,7 @@ async function readManifest(path: string, what: string): Promise=7 reconciles the edited - // package.json against the existing lockfile incrementally, so we never delete node_modules - // or the lockfile: user-added deps keep their resolved versions and installs stay warm. + // Install when the manifest changed OR node_modules is missing (e.g. the user deleted it, or + // `create --no-install` never populated it). npm >=7 reconciles the edited package.json + // against the existing lockfile incrementally, so we never delete node_modules or the + // lockfile: user-added deps keep their resolved versions and installs stay warm. const needsInstall = hasPlannedChanges || !existsSync(join(projectDir, 'node_modules')); if (!needsInstall) { return result; @@ -141,6 +147,14 @@ export async function syncManagedDependencies(options: SyncManagedDependenciesOp const install = await runSubprocessCapture('npm', ['install'], { cwd: projectDir }); if (install.code !== 0) { + // Restore the pre-rewrite package.json before failing. Without this, the retry would see a + // manifest that already matches the pins and an existing node_modules (a failed install + // doesn't remove it), skip the install, and deploy against the stale installed tree — the + // exact skew this sync exists to prevent. Restoring makes the retry recompute the same plan + // and re-attempt the install. Best-effort: a restore failure must not mask the install error. + if (hasPlannedChanges) { + await writeFile(projectPackageJsonPath, projectRaw, 'utf-8').catch(() => undefined); + } throw new DependencySyncError( `npm install failed after updating managed dependencies (exit ${String(install.code)}): ${install.stderr}` ); From e0260103c7c49f1e71c01fef3d562d95312e2f08 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Fri, 17 Jul 2026 05:14:59 +0000 Subject: [PATCH 5/8] =?UTF-8?q?fix:=20don't=20re-print=20dep-sync=20notice?= =?UTF-8?q?=20on=20failed=20deploys=20=E2=80=94=20onNotice=20already=20pri?= =?UTF-8?q?nted=20it=20live?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/cli/commands/deploy/command.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/cli/commands/deploy/command.tsx b/src/cli/commands/deploy/command.tsx index a7f527e6f..066d3bfd1 100644 --- a/src/cli/commands/deploy/command.tsx +++ b/src/cli/commands/deploy/command.tsx @@ -62,12 +62,10 @@ async function handleDeployCLI(options: DeployOptions): Promise { if (options.json) { console.log(JSON.stringify(serializeResult(deployResult))); } else { - // Dependency sync outcome (#1540) still matters on a failed deploy: the notice explains - // the package.json rewrite that DID happen, and a downgraded-skew warning may explain the - // failure itself. printDeployResult only runs on success, so print them here. - if (deployResult.dependencySync?.notice) { - console.error(`${deployResult.dependencySync.notice}\n`); - } + // Dependency sync warnings (#1540) still matter on a failed deploy: a downgraded-skew + // warning may explain the failure itself, and printDeployResult only runs on success. + // (The sync notice is NOT re-printed here — onNotice already printed it live during the + // sync step for every non-JSON run.) for (const warning of deployResult.dependencySync?.warnings ?? []) { console.error(`⚠ ${warning}`); } From e9d507cdc27ed1388a57924f96609e1174cbcd54 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Fri, 17 Jul 2026 19:37:04 +0000 Subject: [PATCH 6/8] refactor: rename dep-sync module files and fields per review; add e2e suite - policy.ts -> plan.ts, index.ts -> sync.ts with thin re-export barrel - DependencySyncResult.migrated -> migratedFromCaret (consistent with SyncPlan) - SKIPPED_SYNC_RESULT -> NO_OP_SYNC_RESULT via noOpSyncResult factory - DeployResult.dependencySync -> dependencySyncResult; depSync local -> shorthand - move toDepSyncAttrs into operations/deploy/dependency-sync.ts - readManifest 'what' param -> fileDescription; install -> installResult - drop #1540 issue references from comments - add e2e-tests/dependency-sync.test.ts (migration+real deploy, upgrade, skew failure, opt-out warning) --- e2e-tests/dependency-sync.test.ts | 435 ++++++++++++++++++ src/cli/commands/deploy/actions.ts | 46 +- src/cli/commands/deploy/command.tsx | 17 +- src/cli/commands/deploy/progress.ts | 10 +- src/cli/commands/deploy/types.ts | 2 +- src/cli/commands/deploy/utils.ts | 15 - src/cli/operations/deploy/dependency-sync.ts | 55 ++- src/cli/operations/deploy/index.ts | 3 +- .../tui/hooks/__tests__/useDevDeploy.test.tsx | 2 +- src/cli/tui/hooks/useCdkPreflight.ts | 4 +- src/cli/tui/hooks/useDevDeploy.ts | 10 +- src/cli/tui/screens/deploy/DeployScreen.tsx | 4 +- src/cli/tui/screens/deploy/useDeployFlow.ts | 9 +- src/cli/tui/screens/dev/DevScreen.tsx | 4 +- .../{policy.test.ts => plan.test.ts} | 2 +- .../__tests__/sync.test.ts | 6 +- src/lib/dependency-management/index.ts | 168 +------ src/lib/dependency-management/messages.ts | 10 +- .../{policy.ts => plan.ts} | 0 src/lib/dependency-management/sync.ts | 167 +++++++ src/lib/dependency-management/types.ts | 2 +- 21 files changed, 709 insertions(+), 262 deletions(-) create mode 100644 e2e-tests/dependency-sync.test.ts rename src/lib/dependency-management/__tests__/{policy.test.ts => plan.test.ts} (99%) rename src/lib/dependency-management/{policy.ts => plan.ts} (100%) create mode 100644 src/lib/dependency-management/sync.ts diff --git a/e2e-tests/dependency-sync.test.ts b/e2e-tests/dependency-sync.test.ts new file mode 100644 index 000000000..d434d0183 --- /dev/null +++ b/e2e-tests/dependency-sync.test.ts @@ -0,0 +1,435 @@ +import { parseJsonOutput, spawnAndCollect } from '../src/test-utils/index.js'; +import { + baseCanRun as canRun, + hasAws, + installCdkTarball, + runAgentCoreCLI, + teardownE2EProject, + writeAwsTargets, +} from './e2e-helper.js'; +import { dumpFailureContext } from './utils/failure-context.js'; +import { randomUUID } from 'node:crypto'; +import { mkdir, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * E2E tests for the managed-dependency sync deploy preflight (#1540 / PR #1777). + * + * The unit tests in src/lib/dependency-management/__tests__/ cover the sync logic with the npm + * subprocess mocked. These tests exercise the wiring through the real CLI binary — option + * passing, JSON envelope shape, exit codes — and (test 1 only) a REAL `npm install` against a + * real node_modules, surviving a real CDK deploy. Only test 1 creates a stack; tests 2-4 stop + * before any CloudFormation work happens. + * + * Each suite is fully independent (own project, own beforeAll/afterAll) per the e2e convention. + */ + +/** The managed dependency these tests manipulate. Verified against the vended manifest at runtime. */ +const MANAGED_DEP = 'constructs'; +/** Second managed dep migrated in test 1 (also tilde-pinned in the vended asset). */ +const SECOND_MANAGED_DEP = 'aws-cdk-lib'; +/** User-owned dependency the sync must never touch. */ +const USER_DEP = 'lodash'; +const USER_DEP_SPEC = '^4.17.21'; + +/** Shape of the dependency-sync outcome inside the `deploy --json` envelope (DependencySyncResult). */ +interface DepSyncJson { + optedOut: boolean; + checkOnly: boolean; + migratedFromCaret: boolean; + reinstalled: boolean; + skewWarning: boolean; + changes: { name: string; section: string; from: string; to: string }[]; + restored: { name: string; section: string; to: string }[]; + skipped: { name: string; raw: string; reason: string }[]; + warnings: string[]; + notice: string | null; +} + +interface DeployJsonEnvelope { + success: boolean; + error?: string; + errorName?: string; + dependencySyncResult?: DepSyncJson; +} + +interface CdkManifest { + dependencies?: Record; + devDependencies?: Record; + [key: string]: unknown; +} + +function cdkPackageJsonPath(projectPath: string): string { + return join(projectPath, 'agentcore', 'cdk', 'package.json'); +} + +async function readCdkManifest(projectPath: string): Promise<{ manifest: CdkManifest; raw: string }> { + const raw = await readFile(cdkPackageJsonPath(projectPath), 'utf-8'); + return { manifest: JSON.parse(raw) as CdkManifest, raw }; +} + +/** Read-modify-write one dependency specifier in agentcore/cdk/package.json. */ +async function setManagedDepVersion(projectPath: string, name: string, spec: string): Promise { + const { manifest } = await readCdkManifest(projectPath); + manifest.dependencies ??= {}; + manifest.dependencies[name] = spec; + await writeFile(cdkPackageJsonPath(projectPath), JSON.stringify(manifest, null, 2) + '\n', 'utf-8'); +} + +/** Extract the base major.minor.patch from a tilde/caret/exact specifier. */ +function baseVersionOf(spec: string): { major: number; minor: number; patch: number } { + const match = /^[~^]?(\d+)\.(\d+)\.(\d+)/.exec(spec); + if (!match) throw new Error(`Cannot parse managed dependency specifier "${spec}"`); + return { major: Number(match[1]), minor: Number(match[2]), patch: Number(match[3]) }; +} + +/** A version strictly lower than the given spec's base — a routine in-range upgrade, never skew. */ +function lowerVersionOf(spec: string): string { + const v = baseVersionOf(spec); + if (v.patch > 0) return `${v.major}.${v.minor}.${v.patch - 1}`; + if (v.minor > 0) return `${v.major}.${v.minor - 1}.0`; + return `${v.major - 1}.0.0`; +} + +/** A higher-minor version than the given spec's base — the newer-than-CLI skew case. */ +function higherMinorVersionOf(spec: string): string { + const v = baseVersionOf(spec); + return `${v.major}.${v.minor + 2}.0`; +} + +/** Rewrite any specifier to a caret range over the same base (simulates a pre-pinning project). */ +function toCaret(spec: string): string { + return `^${spec.replace(/^[~^]/, '')}`; +} + +/** + * Scaffold a real project the way createE2ESuite does: `create` → write aws-targets.json → + * install the CDK tarball override when CDK_TARBALL is set (CI builds). + */ +async function scaffoldProject(testDir: string, agentName: string): Promise { + const result = await runAgentCoreCLI( + [ + 'create', + '--name', + agentName, + '--language', + 'Python', + '--framework', + 'Strands', + '--model-provider', + 'Bedrock', + '--memory', + 'none', + '--json', + ], + testDir + ); + expect(result.exitCode, `Create failed: stderr=${result.stderr}\n\nstdout=${result.stdout}`).toBe(0); + const json = parseJsonOutput(result.stdout) as { projectPath: string }; + await writeAwsTargets(json.projectPath); + installCdkTarball(json.projectPath); + return json.projectPath; +} + +// ───────────────────────────────────────────────────────────────────────────── +// Test 1 — caret migration + real npm install + user-dep preservation +// The only test in this suite that performs a full real deploy (and destroy). +// ───────────────────────────────────────────────────────────────────────────── + +describe.sequential('e2e: dependency sync — caret migration survives a real deploy', () => { + let testDir: string; + let projectPath: string; + const agentName = `E2eDepSyncMig${randomUUID().replace(/-/g, '').slice(0, 8)}`; + let vendedManagedSpec: string; + let vendedSecondSpec: string; + + beforeAll(async () => { + if (!canRun) return; + + testDir = join(tmpdir(), `agentcore-e2e-depsync-${randomUUID()}`); + await mkdir(testDir, { recursive: true }); + projectPath = await scaffoldProject(testDir, agentName); + + // Capture the vended specifiers BEFORE editing — they are the sync's source of truth. + const { manifest } = await readCdkManifest(projectPath); + vendedManagedSpec = manifest.dependencies?.[MANAGED_DEP] ?? ''; + vendedSecondSpec = manifest.dependencies?.[SECOND_MANAGED_DEP] ?? ''; + expect(vendedManagedSpec, `${MANAGED_DEP} should be a managed (vended) dependency`).toMatch(/^~/); + expect(vendedSecondSpec, `${SECOND_MANAGED_DEP} should be a managed (vended) dependency`).toMatch(/^~/); + + // Simulate a pre-pinning project: caret ranges on managed deps (one on a lower base, + // one on the same base) plus a user-owned dependency the CLI doesn't manage. + await setManagedDepVersion(projectPath, MANAGED_DEP, `^${lowerVersionOf(vendedManagedSpec)}`); + await setManagedDepVersion(projectPath, SECOND_MANAGED_DEP, toCaret(vendedSecondSpec)); + await setManagedDepVersion(projectPath, USER_DEP, USER_DEP_SPEC); + }, 300000); + + // Always destroy AWS resources — never skip this + afterAll(async () => { + if (projectPath && hasAws) { + await teardownE2EProject(projectPath, agentName, 'Bedrock'); + } + if (testDir) await rm(testDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }); + }, 600000); + + it.skipIf(!canRun)( + 'migrates caret pins, reinstalls, preserves user deps, and deploys', + async () => { + expect(projectPath, 'Project should have been created').toBeTruthy(); + + const result = await runAgentCoreCLI(['deploy', '--yes', '--json'], projectPath); + if (result.exitCode !== 0) { + await dumpFailureContext({ + label: 'deploy (dependency-sync migration)', + result, + cwd: projectPath, + stackName: `AgentCore-${agentName}-default`, + }); + } + expect(result.exitCode, `Deploy failed (stderr: ${result.stderr}, stdout: ${result.stdout})`).toBe(0); + + const json = parseJsonOutput(result.stdout) as DeployJsonEnvelope; + expect(json.success, 'Deploy should report success').toBe(true); + + // The sync outcome rides on the JSON envelope as dependencySyncResult. + const sync = json.dependencySyncResult; + expect(sync, 'Deploy result should carry dependencySyncResult').toBeDefined(); + expect(sync!.migratedFromCaret, 'Caret ranges should be detected as a pre-pinning migration').toBe(true); + expect(sync!.reinstalled, 'npm install should have run to reconcile the rewritten manifest').toBe(true); + expect(sync!.notice, 'Migration should produce a user-facing notice').toBeTruthy(); + const changedNames = sync!.changes.map(c => c.name); + expect(changedNames).toContain(MANAGED_DEP); + expect(changedNames).toContain(SECOND_MANAGED_DEP); + + // Managed deps are back on the vended specifiers — no more carets. + const { manifest } = await readCdkManifest(projectPath); + expect(manifest.dependencies?.[MANAGED_DEP]).toBe(vendedManagedSpec); + expect(manifest.dependencies?.[SECOND_MANAGED_DEP]).toBe(vendedSecondSpec); + + // The user-owned dependency is untouched at its original range. + expect(manifest.dependencies?.[USER_DEP]).toBe(USER_DEP_SPEC); + + // Spot-check the real installed tree: node_modules/constructs resolves within the pinned range. + const installedRaw = await readFile( + join(projectPath, 'agentcore', 'cdk', 'node_modules', MANAGED_DEP, 'package.json'), + 'utf-8' + ); + const installed = baseVersionOf((JSON.parse(installedRaw) as { version: string }).version); + const pinned = baseVersionOf(vendedManagedSpec); + expect(installed.major, `${MANAGED_DEP} major should match the pinned range`).toBe(pinned.major); + expect(installed.minor, `${MANAGED_DEP} minor should match the tilde-pinned range`).toBe(pinned.minor); + expect(installed.patch, `${MANAGED_DEP} patch should satisfy the tilde-pinned range`).toBeGreaterThanOrEqual( + pinned.patch + ); + }, + 600000 + ); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Test 2 — in-range lower version becomes a routine upgrade, reported by --dry-run +// Cheap: reaches the sync step and synth, but never bootstraps or deploys. +// ───────────────────────────────────────────────────────────────────────────── + +describe.sequential('e2e: dependency sync — dry-run reports an in-range upgrade without writing', () => { + let testDir: string; + let projectPath: string; + const agentName = `E2eDepSyncUp${randomUUID().replace(/-/g, '').slice(0, 8)}`; + let vendedManagedSpec: string; + let staleSpec: string; + let rawAfterEdit: string; + + beforeAll(async () => { + if (!canRun) return; + + testDir = join(tmpdir(), `agentcore-e2e-depsync-${randomUUID()}`); + await mkdir(testDir, { recursive: true }); + projectPath = await scaffoldProject(testDir, agentName); + + const { manifest } = await readCdkManifest(projectPath); + vendedManagedSpec = manifest.dependencies?.[MANAGED_DEP] ?? ''; + expect(vendedManagedSpec, `${MANAGED_DEP} should be a managed (vended) dependency`).toMatch(/^~/); + + // Lower-than-vended base version: an ordinary upgrade on next deploy, NOT skew. + staleSpec = `~${lowerVersionOf(vendedManagedSpec)}`; + await setManagedDepVersion(projectPath, MANAGED_DEP, staleSpec); + rawAfterEdit = (await readCdkManifest(projectPath)).raw; + }, 300000); + + afterAll(async () => { + // No stack was ever created — only the local project needs cleaning up. + if (testDir) await rm(testDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }); + }, 30000); + + it.skipIf(!canRun)( + 'dry-run reports the pending version bump and leaves package.json unchanged', + async () => { + expect(projectPath, 'Project should have been created').toBeTruthy(); + + const result = await runAgentCoreCLI(['deploy', '--dry-run', '--yes', '--json'], projectPath); + expect(result.exitCode, `Dry-run failed (stderr: ${result.stderr}, stdout: ${result.stdout})`).toBe(0); + + const json = parseJsonOutput(result.stdout) as DeployJsonEnvelope; + expect(json.success).toBe(true); + + const sync = json.dependencySyncResult; + expect(sync, 'Dry-run result should carry dependencySyncResult').toBeDefined(); + expect(sync!.checkOnly, 'Preview mode should run the sync check-only').toBe(true); + expect(sync!.reinstalled).toBe(false); + expect(sync!.changes).toContainEqual({ + name: MANAGED_DEP, + section: 'dependencies', + from: staleSpec, + to: vendedManagedSpec, + }); + expect(sync!.notice, 'Dry-run should report the pending bump in the notice').toBeTruthy(); + expect(sync!.notice).toContain(staleSpec); + expect(sync!.notice).toContain(vendedManagedSpec); + + // The whole preview-mode design hinges on this: --dry-run must never mutate the working tree. + const { raw } = await readCdkManifest(projectPath); + expect(raw, 'package.json must be untouched by --dry-run').toBe(rawAfterEdit); + }, + 300000 + ); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Test 3 — newer-than-CLI skew blocks a real deploy with the upgrade-CLI error +// Cheap: CliVersionTooOldError is thrown inside the sync step itself, before the +// CDK build/synth ever run — no stack is created. Note this must be a REAL deploy +// (not --dry-run): preview mode deliberately downgrades skew to a warning, so the +// hard failure is only observable on a mutating deploy. +// ───────────────────────────────────────────────────────────────────────────── + +describe.sequential('e2e: dependency sync — newer-than-CLI skew fails fast with the upgrade error', () => { + let testDir: string; + let projectPath: string; + const agentName = `E2eDepSyncSkw${randomUUID().replace(/-/g, '').slice(0, 8)}`; + let rawAfterEdit: string; + // Defensive: if the skew guard ever regresses and the deploy goes through, tear the stack down. + let deployedUnexpectedly = false; + + beforeAll(async () => { + if (!canRun) return; + + testDir = join(tmpdir(), `agentcore-e2e-depsync-${randomUUID()}`); + await mkdir(testDir, { recursive: true }); + projectPath = await scaffoldProject(testDir, agentName); + + const { manifest } = await readCdkManifest(projectPath); + const vendedManagedSpec = manifest.dependencies?.[MANAGED_DEP] ?? ''; + expect(vendedManagedSpec, `${MANAGED_DEP} should be a managed (vended) dependency`).toMatch(/^~/); + + // Higher minor than the vended pin: the project was touched by a newer CLI. + await setManagedDepVersion(projectPath, MANAGED_DEP, `~${higherMinorVersionOf(vendedManagedSpec)}`); + rawAfterEdit = (await readCdkManifest(projectPath)).raw; + }, 300000); + + afterAll(async () => { + if (deployedUnexpectedly && projectPath && hasAws) { + await teardownE2EProject(projectPath, agentName, 'Bedrock'); + } + if (testDir) await rm(testDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }); + }, 600000); + + it.skipIf(!canRun)( + 'deploy exits 1 with the upgrade-CLI error and never writes package.json', + async () => { + expect(projectPath, 'Project should have been created').toBeTruthy(); + + const result = await runAgentCoreCLI(['deploy', '--yes', '--json'], projectPath); + deployedUnexpectedly = result.exitCode === 0; + + expect(result.exitCode, `Skewed deploy should fail (stdout: ${result.stdout})`).toBe(1); + + const json = parseJsonOutput(result.stdout) as DeployJsonEnvelope; + expect(json.success).toBe(false); + // Distinctive substrings from formatCliUpgradeError, not the full copy. + expect(json.error).toContain('requires a newer version of the AgentCore CLI'); + expect(json.error).toContain(MANAGED_DEP); + expect(json.errorName).toBe('CliVersionTooOldError'); + + // Skew is detected before anything is written — the manifest must be untouched. + const { raw } = await readCdkManifest(projectPath); + expect(raw, 'package.json must be untouched by a skew failure').toBe(rawAfterEdit); + }, + 180000 + ); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Test 4 — the global opt-out downgrades skew to a warning +// Cheap: same shape as test 2. Uses an isolated AGENTCORE_CONFIG_DIR so the +// `agentcore config` write can't leak into the machine's real global config. +// ───────────────────────────────────────────────────────────────────────────── + +describe.sequential('e2e: dependency sync — opt-out downgrades skew to a warning', () => { + let testDir: string; + let projectPath: string; + let configDir: string; + const agentName = `E2eDepSyncOpt${randomUUID().replace(/-/g, '').slice(0, 8)}`; + let rawAfterEdit: string; + + beforeAll(async () => { + if (!canRun) return; + + testDir = join(tmpdir(), `agentcore-e2e-depsync-${randomUUID()}`); + await mkdir(testDir, { recursive: true }); + projectPath = await scaffoldProject(testDir, agentName); + + // Opt out of dependency management, isolated from the real ~/.agentcore. + configDir = join(testDir, 'agentcore-global-config'); + const configResult = await spawnAndCollect( + 'agentcore', + ['config', 'disableDependencyManagement', 'true'], + projectPath, + { AGENTCORE_CONFIG_DIR: configDir } + ); + expect(configResult.exitCode, `config set failed: ${configResult.stderr}`).toBe(0); + expect(configResult.stdout).toContain('Set disableDependencyManagement = true'); + + // Same higher-minor skew as test 3. + const { manifest } = await readCdkManifest(projectPath); + const vendedManagedSpec = manifest.dependencies?.[MANAGED_DEP] ?? ''; + expect(vendedManagedSpec, `${MANAGED_DEP} should be a managed (vended) dependency`).toMatch(/^~/); + await setManagedDepVersion(projectPath, MANAGED_DEP, `~${higherMinorVersionOf(vendedManagedSpec)}`); + rawAfterEdit = (await readCdkManifest(projectPath)).raw; + }, 300000); + + afterAll(async () => { + // No stack was ever created — only the local project needs cleaning up. + if (testDir) await rm(testDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }); + }, 30000); + + it.skipIf(!canRun)( + 'deploy proceeds with a skew warning and leaves package.json unchanged', + async () => { + expect(projectPath, 'Project should have been created').toBeTruthy(); + + const result = await spawnAndCollect('agentcore', ['deploy', '--dry-run', '--yes', '--json'], projectPath, { + AGENTCORE_CONFIG_DIR: configDir, + }); + expect(result.exitCode, `Opted-out deploy failed (stderr: ${result.stderr}, stdout: ${result.stdout})`).toBe(0); + + const json = parseJsonOutput(result.stdout) as DeployJsonEnvelope; + expect(json.success).toBe(true); + + const sync = json.dependencySyncResult; + expect(sync, 'Deploy result should carry dependencySyncResult').toBeDefined(); + expect(sync!.optedOut, 'Global opt-out should be reflected in the sync result').toBe(true); + expect(sync!.skewWarning, 'Skew should be downgraded to a warning when opted out').toBe(true); + const warnings = sync!.warnings.join('\n'); + expect(warnings).toContain(MANAGED_DEP); + expect(warnings).toContain('newer than this CLI was tested with'); + expect(warnings).toContain('upgrade the CLI'); + + // Opted-out sync never writes. + const { raw } = await readCdkManifest(projectPath); + expect(raw, 'package.json must be untouched when opted out').toBe(rawAfterEdit); + }, + 300000 + ); +}); diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index cdfa067a6..996887345 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -182,7 +182,7 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { currentStepName = name; @@ -276,7 +276,7 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise 0 ? allWarnings : undefined, - dependencySync: depSync, + dependencySyncResult, }; } catch (err: unknown) { logger.log(getErrorMessage(err), 'error'); logger.finalize(false); // Carry the dep-sync outcome on the failure result too, so dep_sync_* telemetry // is recorded even when a later step throws. - return { success: false, error: toError(err), logPath: logger.getRelativeLogPath(), dependencySync: depSync }; + return { success: false, error: toError(err), logPath: logger.getRelativeLogPath(), dependencySyncResult }; } finally { // Each cleanup step must run regardless of whether an earlier one fails — a throw from // dispose() (common after a synth/bootstrap failure on a creds-less preview) must not skip the diff --git a/src/cli/commands/deploy/command.tsx b/src/cli/commands/deploy/command.tsx index 066d3bfd1..820d690ff 100644 --- a/src/cli/commands/deploy/command.tsx +++ b/src/cli/commands/deploy/command.tsx @@ -1,12 +1,13 @@ import { ConfigIO, serializeResult } from '../../../lib'; import { COMMAND_DESCRIPTIONS } from '../../constants'; import { getErrorMessage } from '../../errors'; +import { toDepSyncAttrs } from '../../operations/deploy'; import { withCommandRunTelemetry } from '../../telemetry/cli-command-run.js'; import { renderTUI } from '../../tui'; import { requireProject, requireTTY } from '../../tui/guards'; import { handleDeploy } from './actions'; import type { DeployOptions, DeployResult } from './types'; -import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs, toDepSyncAttrs } from './utils'; +import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs } from './utils'; import { validateDeployOptions } from './validate'; import type { Command } from '@commander-js/extra-typings'; import { Text, render } from 'ink'; @@ -48,8 +49,8 @@ async function handleDeployCLI(options: DeployOptions): Promise { ); // Record dep_sync attrs whenever the sync ran — including on failed deploys, where the // failure result still carries the outcome (see handleDeploy's catch). - if (result.dependencySync) { - recorder.set(toDepSyncAttrs(result.dependencySync)); + if (result.dependencySyncResult) { + recorder.set(toDepSyncAttrs(result.dependencySyncResult)); } if (!result.success) { return { success: false as const, error: result.error, deployResult: result }; @@ -62,11 +63,11 @@ async function handleDeployCLI(options: DeployOptions): Promise { if (options.json) { console.log(JSON.stringify(serializeResult(deployResult))); } else { - // Dependency sync warnings (#1540) still matter on a failed deploy: a downgraded-skew + // Dependency sync warnings still matter on a failed deploy: a downgraded-skew // warning may explain the failure itself, and printDeployResult only runs on success. // (The sync notice is NOT re-printed here — onNotice already printed it live during the // sync step for every non-JSON run.) - for (const warning of deployResult.dependencySync?.warnings ?? []) { + for (const warning of deployResult.dependencySyncResult?.warnings ?? []) { console.error(`⚠ ${warning}`); } console.error(deployResult.error.message); @@ -158,11 +159,11 @@ function printDeployResult(result: DeployResult & { success: true }, options: De return; } - // Managed dependency sync warnings (#1540): downgraded skew, skipped specifiers. Informational + // Dependency sync warnings: downgraded skew, skipped specifiers. Informational // only — they must reach the terminal but must NOT flip the exit code to 2 (the bundled-tarball // override in e2e/dev builds is always "left unmanaged", and exit 2 would fail every deploy). - if (result.dependencySync?.warnings && result.dependencySync.warnings.length > 0) { - for (const warning of result.dependencySync.warnings) { + if (result.dependencySyncResult?.warnings && result.dependencySyncResult.warnings.length > 0) { + for (const warning of result.dependencySyncResult.warnings) { console.warn(`⚠ ${warning}`); } } diff --git a/src/cli/commands/deploy/progress.ts b/src/cli/commands/deploy/progress.ts index d73b77038..09ec1cf4b 100644 --- a/src/cli/commands/deploy/progress.ts +++ b/src/cli/commands/deploy/progress.ts @@ -68,12 +68,12 @@ export async function runCliDeploy(): Promise { }); cleanup(); - // Surface the managed-dependency sync outcome (#1540) — handleDeploy only writes it to the - // deploy log; this dev path passes no onNotice, so print it here. - if (result.dependencySync?.notice) { - console.log(`\n${result.dependencySync.notice}\n`); + // Print the dependency sync notice and warnings — handleDeploy only writes them to the + // deploy log, and this dev path passes no onNotice. + if (result.dependencySyncResult?.notice) { + console.log(`\n${result.dependencySyncResult.notice}\n`); } - for (const warning of result.dependencySync?.warnings ?? []) { + for (const warning of result.dependencySyncResult?.warnings ?? []) { console.warn(`${ANSI.yellow}⚠ ${warning}${ANSI.reset}`); } diff --git a/src/cli/commands/deploy/types.ts b/src/cli/commands/deploy/types.ts index d2a20d7be..d0df81a5e 100644 --- a/src/cli/commands/deploy/types.ts +++ b/src/cli/commands/deploy/types.ts @@ -21,7 +21,7 @@ export type DeployResult = Result<{ }> & { logPath?: string; /** Sync outcome rides on BOTH branches so dep_sync_* telemetry survives a failed deploy. */ - dependencySync?: DependencySyncResult; + dependencySyncResult?: DependencySyncResult; }; export type PreflightResult = Result<{ diff --git a/src/cli/commands/deploy/utils.ts b/src/cli/commands/deploy/utils.ts index ea95becb6..d6362e114 100644 --- a/src/cli/commands/deploy/utils.ts +++ b/src/cli/commands/deploy/utils.ts @@ -1,4 +1,3 @@ -import type { DependencySyncResult } from '../../../lib/dependency-management'; import type { AgentCoreProjectSpec } from '../../../schema'; import type { DeployMode } from '../../telemetry/schemas/common-shapes'; @@ -16,20 +15,6 @@ export const DEFAULT_DEPLOY_ATTRS = { deploy_mode: 'deploy' as DeployMode, }; -/** - * Map a managed-dependency sync outcome (#1540) to its dep_sync_* telemetry attrs. - * Single source for both the CLI command (command.tsx) and the TUI flow (useDeployFlow). - */ -export function toDepSyncAttrs(sync: DependencySyncResult) { - return { - dep_sync_changed_count: sync.changes.length + sync.restored.length, - dep_sync_migrated: sync.migrated, - dep_sync_opted_out: sync.optedOut, - dep_sync_skew_warning: sync.skewWarning, - dep_sync_reinstalled: sync.reinstalled, - }; -} - export function computeDeployAttrs(projectSpec: Partial, mode: DeployMode) { const gateways = projectSpec.agentCoreGateways ?? []; const policyEngines = projectSpec.policyEngines ?? []; diff --git a/src/cli/operations/deploy/dependency-sync.ts b/src/cli/operations/deploy/dependency-sync.ts index e4c4df07c..95ca403a0 100644 --- a/src/cli/operations/deploy/dependency-sync.ts +++ b/src/cli/operations/deploy/dependency-sync.ts @@ -21,19 +21,27 @@ export interface EnsureManagedDependenciesOptions { treatSkewAsWarning?: boolean; } +/** + * A DependencySyncResult in which no dependency operation took effect: nothing was + * written, installed, or warned about. Base shape for the no-op states below. + */ +function noOpSyncResult(warnings: string[] = []): DependencySyncResult { + return { + optedOut: false, + checkOnly: true, + migratedFromCaret: false, + reinstalled: false, + skewWarning: false, + changes: [], + restored: [], + skipped: [], + warnings, + notice: null, + }; +} + /** No-op result for when the sync is skipped entirely (AGENTCORE_SKIP_INSTALL). */ -const SKIPPED_SYNC_RESULT: DependencySyncResult = { - optedOut: false, - checkOnly: true, - migrated: false, - reinstalled: false, - skewWarning: false, - changes: [], - restored: [], - skipped: [], - warnings: [], - notice: null, -}; +const NO_OP_SYNC_RESULT: DependencySyncResult = noOpSyncResult(); /** * Warning-only result for a DependencySyncError caught on a teardown deploy. Teardown must @@ -43,14 +51,11 @@ const SKIPPED_SYNC_RESULT: DependencySyncResult = { * node_modules is genuinely unusable, the build step fails on its own. */ export function teardownSyncFailureResult(err: Error): DependencySyncResult { - return { - ...SKIPPED_SYNC_RESULT, - warnings: [`Dependency sync failed (continuing with teardown): ${err.message}`], - }; + return noOpSyncResult([`Dependency sync failed (continuing with teardown): ${err.message}`]); } /** - * Deploy-preflight entry point for managed dependency pinning (#1540): resolves the + * Deploy-preflight entry point for managed dependency pinning: resolves the * CLI's vended CDK package.json (source of truth), the global opt-out, and the * distro-aware CLI install command, then runs the sync against the project's * agentcore/cdk directory. A future `agentcore build` command should call this same @@ -60,12 +65,26 @@ export function teardownSyncFailureResult(err: Error): DependencySyncResult { * Throws CliVersionTooOldError when the project was updated by a newer CLI (unless * `treatSkewAsWarning` or opted out), and DependencySyncError on rewrite/install failure. */ +/** + * Map a dependency sync result to its dep_sync_* telemetry attrs. + * Single source for both the CLI command (command.tsx) and the TUI flow (useDeployFlow). + */ +export function toDepSyncAttrs(sync: DependencySyncResult) { + return { + dep_sync_changed_count: sync.changes.length + sync.restored.length, + dep_sync_migrated: sync.migratedFromCaret, + dep_sync_opted_out: sync.optedOut, + dep_sync_skew_warning: sync.skewWarning, + dep_sync_reinstalled: sync.reinstalled, + }; +} + export async function ensureManagedDependencies( cdkProject: LocalCdkProject, options: EnsureManagedDependenciesOptions = {} ): Promise { if (process.env.AGENTCORE_SKIP_INSTALL) { - return SKIPPED_SYNC_RESULT; + return NO_OP_SYNC_RESULT; } const disabled = readGlobalConfigSync().disableDependencyManagement === true; return syncManagedDependencies({ diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index 3be5ba91d..0bba391cb 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -68,10 +68,11 @@ export { ensureDefaultDeploymentTarget } from './ensure-target'; // Pre-synth backfill of vpcId for pre-existing Container+VPC configs written before vpcId was added export { backfillContainerVpcIds, type BackfillVpcIdResult } from './backfill-vpc-id'; -// Managed dependency pinning (#1540) — shared by CLI deploy + TUI preflight +// Managed dependency pinning — shared by CLI deploy + TUI preflight export { ensureManagedDependencies, teardownSyncFailureResult, + toDepSyncAttrs, SYNC_CDK_DEPENDENCIES_STEP, type DependencySyncResult, type EnsureManagedDependenciesOptions, diff --git a/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx b/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx index b81e8d9eb..2613df818 100644 --- a/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx +++ b/src/cli/tui/hooks/__tests__/useDevDeploy.test.tsx @@ -149,7 +149,7 @@ describe('useDevDeploy', () => { it('reads the dependency sync notice and warnings from the deploy result', async () => { mockHandleDeploy.mockResolvedValue({ success: true, - dependencySync: { + dependencySyncResult: { notice: 'dep-sync-notice', warnings: ['lodash (file:x) uses a non-semver specifier and was left unmanaged.'], }, diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index c3469a3c3..7a124896c 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -172,7 +172,7 @@ export interface PreflightResult { missingCredentials: MissingCredential[]; /** KMS key ARN used for identity token vault encryption */ identityKmsKeyArn?: string; - /** Managed dependency sync outcome (#1540) — notice/warnings for display, attrs for telemetry */ + /** Result of the managed dependency sync — notice/warnings for display, attrs for telemetry */ dependencySync: DependencySyncResult | null; /** Credential ARNs (API key + OAuth) from pre-deploy setup */ allCredentials: Record; @@ -500,7 +500,7 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { return; } - // Step: Sync managed dependencies (#1540) — pin agentcore/cdk/package.json to the versions + // Step: Sync managed dependencies — pin agentcore/cdk/package.json to the versions // this CLI was tested with, migrating pre-pinning (caret) projects. Runs before the build so // the compile sees the reinstalled tree. Diff mode runs check-only (never mutates the // working tree), and teardown deploys downgrade every sync failure to a warning: skew via diff --git a/src/cli/tui/hooks/useDevDeploy.ts b/src/cli/tui/hooks/useDevDeploy.ts index 2fbaf15d0..ad42f5d44 100644 --- a/src/cli/tui/hooks/useDevDeploy.ts +++ b/src/cli/tui/hooks/useDevDeploy.ts @@ -20,9 +20,9 @@ export interface UseDevDeployResult { logPath: string | undefined; /** Managed-memory heads-up surfaced by handleDeploy (null when not applicable) */ managedMemoryNotice: string | null; - /** Managed dependency sync summary (#1540) from the deploy result (null when nothing changed) */ + /** Managed dependency sync summary from the deploy result (null when nothing changed) */ dependencySyncNotice: string | null; - /** Managed dependency sync warnings (#1540) from the deploy result */ + /** Managed dependency sync warnings from the deploy result */ dependencySyncWarnings: string[]; } @@ -106,9 +106,9 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): setLogPath(result.logPath); } - if (result.dependencySync) { - setDependencySyncNotice(result.dependencySync.notice); - setDependencySyncWarnings(result.dependencySync.warnings); + if (result.dependencySyncResult) { + setDependencySyncNotice(result.dependencySyncResult.notice); + setDependencySyncWarnings(result.dependencySyncResult.warnings); } if (!result.success) { diff --git a/src/cli/tui/screens/deploy/DeployScreen.tsx b/src/cli/tui/screens/deploy/DeployScreen.tsx index fb958084c..e3c26d58e 100644 --- a/src/cli/tui/screens/deploy/DeployScreen.tsx +++ b/src/cli/tui/screens/deploy/DeployScreen.tsx @@ -346,14 +346,14 @@ export function DeployScreen({ <> - {/* Managed dependency sync summary (#1540): what preflight changed in agentcore/cdk/package.json. */} + {/* Managed dependency sync summary: what preflight changed in agentcore/cdk/package.json. */} {dependencySyncNotice && ( {dependencySyncNotice} )} - {/* Managed dependency sync warnings (#1540): downgraded skew, skipped specifiers. */} + {/* Managed dependency sync warnings: downgraded skew, skipped specifiers. */} {dependencySyncWarnings.length > 0 && ( {dependencySyncWarnings.map((warning, i) => ( diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index 3d0db9142..7bf07645d 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -19,7 +19,7 @@ import { parsePolicyOutputs, parseRuntimeEndpointOutputs, } from '../../../cloudformation'; -import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs, toDepSyncAttrs } from '../../../commands/deploy/utils.js'; +import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs } from '../../../commands/deploy/utils.js'; import { toStackName } from '../../../commands/import/import-utils'; import { getErrorMessage, isChangesetInProgressError, isExpiredTokenError } from '../../../errors'; import { ExecLogger } from '../../../logging'; @@ -29,6 +29,7 @@ import { hasManagedMemoryHarness, performStackTeardown, setupTransactionSearch, + toDepSyncAttrs, } from '../../../operations/deploy'; import { computeProjectDeployHash } from '../../../operations/deploy/change-detection'; import { getGatewayTargetStatuses } from '../../../operations/deploy/gateway-status'; @@ -116,9 +117,9 @@ interface DeployFlowState { deployNotes: string[]; /** Managed-memory heads-up, shown while the CFN apply runs (null when not applicable) */ managedMemoryNotice: string | null; - /** Managed dependency sync summary from preflight (#1540), null when nothing changed */ + /** Managed dependency sync summary from preflight, null when nothing changed */ dependencySyncNotice: string | null; - /** Managed dependency sync warnings (downgraded skew, skipped specifiers) from preflight (#1540) */ + /** Managed dependency sync warnings (downgraded skew, skipped specifiers) from preflight */ dependencySyncWarnings: string[]; /** Warnings from post-deploy steps (config bundles, AB tests) */ postDeployWarnings: string[]; @@ -145,7 +146,7 @@ interface DeployFlowState { skipCredentials: () => void; } -/** Overlay dep_sync_* telemetry attrs from the preflight dependency sync (#1540), if it ran. */ +/** Overlay dep_sync_* telemetry attrs from the preflight dependency sync, if it ran. */ function withDepSyncAttrs(attrs: T, sync: DependencySyncResult | null): T { if (!sync) return attrs; return { ...attrs, ...toDepSyncAttrs(sync) }; diff --git a/src/cli/tui/screens/dev/DevScreen.tsx b/src/cli/tui/screens/dev/DevScreen.tsx index f2785b38e..29e5d232c 100644 --- a/src/cli/tui/screens/dev/DevScreen.tsx +++ b/src/cli/tui/screens/dev/DevScreen.tsx @@ -276,7 +276,7 @@ export function DevScreen(props: DevScreenProps) { if (onLaunchBrowser) { onLaunchBrowser({ harnessName: selectedHarness }); // onLaunchBrowser exits the alt screen and unmounts synchronously, so anything rendered - // in 'deploying' mode is gone. Print the one-shot dep-sync outcome (#1540) — e.g. the + // in 'deploying' mode is gone. Print the one-shot dep-sync notice and warnings — e.g. the // "we rewrote your package.json" migration explanation — to the normal buffer instead. if (dependencySyncNotice) { console.log(`\n${dependencySyncNotice}\n`); @@ -528,7 +528,7 @@ export function DevScreen(props: DevScreenProps) { return null; } - // Managed dependency sync outcome (#1540): its own block — the multi-line notice doesn't fit + // Dependency sync notice and warnings get their own block — the multi-line notice doesn't fit // the single "Note:" line in the deploying view. Rendered during the deploy AND in the // post-deploy modes (harness / select-agent): the deploy completes and the mode transitions in // the same render batch, so a block gated on mode === 'deploying' would be visible for at most diff --git a/src/lib/dependency-management/__tests__/policy.test.ts b/src/lib/dependency-management/__tests__/plan.test.ts similarity index 99% rename from src/lib/dependency-management/__tests__/policy.test.ts rename to src/lib/dependency-management/__tests__/plan.test.ts index 06af5dad5..be2aa5237 100644 --- a/src/lib/dependency-management/__tests__/policy.test.ts +++ b/src/lib/dependency-management/__tests__/plan.test.ts @@ -1,4 +1,4 @@ -import { computeSyncPlan } from '../policy'; +import { computeSyncPlan } from '../plan'; import type { PackageManifest } from '../types'; import { describe, expect, it } from 'vitest'; diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts index f7e9aee37..c535a0848 100644 --- a/src/lib/dependency-management/__tests__/sync.test.ts +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -1,6 +1,6 @@ import { CliVersionTooOldError, DependencySyncError } from '../../errors/types'; -import { syncManagedDependencies } from '../index'; -import type { SyncManagedDependenciesOptions } from '../index'; +import { syncManagedDependencies } from '../sync'; +import type { SyncManagedDependenciesOptions } from '../types'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; @@ -69,7 +69,7 @@ describe('syncManagedDependencies', () => { const result = await run(); - expect(result.migrated).toBe(true); + expect(result.migratedFromCaret).toBe(true); expect(result.reinstalled).toBe(true); expect(result.changes).toHaveLength(2); expect(result.notice).toContain('created before the AgentCore CLI managed dependency versions'); diff --git a/src/lib/dependency-management/index.ts b/src/lib/dependency-management/index.ts index 3c7cc8d2b..2165227b5 100644 --- a/src/lib/dependency-management/index.ts +++ b/src/lib/dependency-management/index.ts @@ -1,18 +1,6 @@ -import { CliVersionTooOldError, DependencySyncError } from '../errors/types'; -import { runSubprocessCapture } from '../utils/subprocess'; -import { - DEFAULT_CLI_INSTALL_COMMAND, - formatCliUpgradeError, - formatSkewWarning, - formatSkippedWarning, - formatSyncNotice, -} from './messages'; -import { computeSyncPlan } from './policy'; -import type { DependencySyncResult, PackageManifest, SyncManagedDependenciesOptions } from './types'; -import { existsSync } from 'node:fs'; -import { readFile, writeFile } from 'node:fs/promises'; -import { join } from 'node:path'; - +export { syncManagedDependencies } from './sync'; +export { computeSyncPlan } from './plan'; +export type { SkewFinding, SyncPlan } from './plan'; export type { DependencyChange, DependencySyncResult, @@ -20,153 +8,3 @@ export type { SkippedDependency, SyncManagedDependenciesOptions, } from './types'; - -async function readManifest(path: string, what: string): Promise<{ manifest: PackageManifest; raw: string }> { - let raw: string; - try { - raw = await readFile(path, 'utf-8'); - } catch (err) { - throw new DependencySyncError(`Could not read ${what} at ${path}: ${String(err)}`, { cause: err }); - } - try { - return { manifest: JSON.parse(raw) as PackageManifest, raw }; - } catch (err) { - throw new DependencySyncError(`Could not parse ${what} at ${path}: ${String(err)}`, { cause: err }); - } -} - -/** - * Sync the managed dependencies of a vended CDK project to the versions this CLI - * release was tested with (agentcore-cli#1540). - * - * The vended asset package.json is the source of truth: every dependency named in it - * is managed and rewritten to its vended specifier (tilde for stable deps so patches - * float, exact for the L3 constructs); everything else in the user's file is untouched. - * Projects that predate pinning (caret ranges) are migrated automatically. If the - * project declares a managed dependency newer than this CLI expects, the project was - * updated by a newer CLI (or the user bumped it manually) and this throws - * CliVersionTooOldError (downgraded to a warning when `disabled` or - * `treatSkewAsWarning`). When anything changed — or node_modules is missing — - * `npm install` reconciles the edited package.json against the existing lockfile - * incrementally; node_modules and the lockfile are never deleted, so user-added deps - * keep their resolved versions. If the install fails, the pre-rewrite package.json is - * restored so a retry recomputes the same plan and re-attempts the install. - * - * `mode: 'check'` computes the same plan/warnings/notice without writing or - * installing, for preview flows that must not mutate the working tree. - * - * All user-facing wording is returned on the result (`notice`, `warnings`) — callers - * only display it. Throws CliVersionTooOldError | DependencySyncError; never exits. - */ -export async function syncManagedDependencies(options: SyncManagedDependenciesOptions): Promise { - const { - vendedPackageJsonPath, - projectDir, - disabled = false, - mode = 'apply', - treatSkewAsWarning = false, - installCommand = DEFAULT_CLI_INSTALL_COMMAND, - } = options; - const checkOnly = mode === 'check'; - const projectPackageJsonPath = join(projectDir, 'package.json'); - - const { manifest: vended } = await readManifest(vendedPackageJsonPath, 'the vended CDK package.json'); - // Keep the raw pre-rewrite text: it's restored verbatim if npm install fails, so a - // retry recomputes the same plan instead of seeing an already-matching manifest. - const { manifest: project, raw: projectRaw } = await readManifest( - projectPackageJsonPath, - "the project's CDK package.json" - ); - - const plan = computeSyncPlan(vended, project); - - const result: DependencySyncResult = { - optedOut: disabled, - checkOnly, - migrated: plan.migratedFromCaret, - reinstalled: false, - skewWarning: false, - changes: plan.changes, - restored: plan.restored, - skipped: plan.skipped, - warnings: plan.skipped.map(formatSkippedWarning), - notice: null, - }; - - if (plan.skew.length > 0) { - if (!disabled && !treatSkewAsWarning && !checkOnly) { - throw new CliVersionTooOldError(formatCliUpgradeError(plan.skew, installCommand)); - } - result.skewWarning = true; - result.warnings.push(formatSkewWarning(plan.skew, installCommand)); - } - - if (disabled) { - return result; - } - - const hasPlannedChanges = plan.changes.length > 0 || plan.restored.length > 0; - - if (checkOnly) { - // Report what WOULD change without touching the working tree (future-tense wording). - result.notice = formatSyncNotice({ - migrated: result.migrated, - changes: result.changes, - restored: result.restored, - reinstalled: false, - applied: false, - }); - return result; - } - - if (hasPlannedChanges) { - // Mutate the parsed manifest in place so user key order and unknown fields survive. - for (const change of plan.changes) { - project[change.section]![change.name] = change.to; - } - for (const restoredDep of plan.restored) { - const section = (project[restoredDep.section] ??= {}); - section[restoredDep.name] = restoredDep.to; - } - - try { - await writeFile(projectPackageJsonPath, JSON.stringify(project, null, 2) + '\n', 'utf-8'); - } catch (err) { - throw new DependencySyncError(`Failed to update the CDK project's dependencies: ${String(err)}`, { cause: err }); - } - } - - // Install when the manifest changed OR node_modules is missing (e.g. the user deleted it, or - // `create --no-install` never populated it). npm >=7 reconciles the edited package.json - // against the existing lockfile incrementally, so we never delete node_modules or the - // lockfile: user-added deps keep their resolved versions and installs stay warm. - const needsInstall = hasPlannedChanges || !existsSync(join(projectDir, 'node_modules')); - if (!needsInstall) { - return result; - } - - const install = await runSubprocessCapture('npm', ['install'], { cwd: projectDir }); - if (install.code !== 0) { - // Restore the pre-rewrite package.json before failing. Without this, the retry would see a - // manifest that already matches the pins and an existing node_modules (a failed install - // doesn't remove it), skip the install, and deploy against the stale installed tree — the - // exact skew this sync exists to prevent. Restoring makes the retry recompute the same plan - // and re-attempt the install. Best-effort: a restore failure must not mask the install error. - if (hasPlannedChanges) { - await writeFile(projectPackageJsonPath, projectRaw, 'utf-8').catch(() => undefined); - } - throw new DependencySyncError( - `npm install failed after updating managed dependencies (exit ${String(install.code)}): ${install.stderr}` - ); - } - result.reinstalled = true; - - result.notice = formatSyncNotice({ - migrated: result.migrated, - changes: result.changes, - restored: result.restored, - reinstalled: result.reinstalled, - applied: true, - }); - return result; -} diff --git a/src/lib/dependency-management/messages.ts b/src/lib/dependency-management/messages.ts index c72a18d3e..159d50852 100644 --- a/src/lib/dependency-management/messages.ts +++ b/src/lib/dependency-management/messages.ts @@ -1,4 +1,4 @@ -import type { SkewFinding } from './policy'; +import type { SkewFinding } from './plan'; import type { DependencyChange, RestoredDependency, SkippedDependency } from './types'; /** @@ -58,18 +58,18 @@ function formatChangeTable(changes: DependencyChange[], restored: RestoredDepend } export function formatSyncNotice(options: { - migrated: boolean; + migratedFromCaret: boolean; changes: DependencyChange[]; restored: RestoredDependency[]; reinstalled: boolean; /** False in check mode — nothing was written, so speak in the future tense. */ applied: boolean; }): string | null { - const { migrated, changes, restored, reinstalled, applied } = options; + const { migratedFromCaret, changes, restored, reinstalled, applied } = options; if (changes.length === 0 && restored.length === 0) return null; const lines: string[] = []; - if (migrated) { + if (migratedFromCaret) { lines.push(applied ? MIGRATION_PREAMBLE : MIGRATION_PREAMBLE_PENDING); } else { lines.push( @@ -89,7 +89,7 @@ export function formatSyncNotice(options: { ? 'Dependencies you added yourself were not changed.' : 'Dependencies you added yourself will not be changed.' ); - if (migrated) { + if (migratedFromCaret) { lines.push(OPT_OUT_HINT); } return lines.join('\n'); diff --git a/src/lib/dependency-management/policy.ts b/src/lib/dependency-management/plan.ts similarity index 100% rename from src/lib/dependency-management/policy.ts rename to src/lib/dependency-management/plan.ts diff --git a/src/lib/dependency-management/sync.ts b/src/lib/dependency-management/sync.ts new file mode 100644 index 000000000..941ac36c9 --- /dev/null +++ b/src/lib/dependency-management/sync.ts @@ -0,0 +1,167 @@ +import { CliVersionTooOldError, DependencySyncError } from '../errors/types'; +import { runSubprocessCapture } from '../utils/subprocess'; +import { + DEFAULT_CLI_INSTALL_COMMAND, + formatCliUpgradeError, + formatSkewWarning, + formatSkippedWarning, + formatSyncNotice, +} from './messages'; +import { computeSyncPlan } from './plan'; +import type { DependencySyncResult, PackageManifest, SyncManagedDependenciesOptions } from './types'; +import { existsSync } from 'node:fs'; +import { readFile, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; + +async function readManifest( + path: string, + fileDescription: string +): Promise<{ manifest: PackageManifest; raw: string }> { + let raw: string; + try { + raw = await readFile(path, 'utf-8'); + } catch (err) { + throw new DependencySyncError(`Could not read ${fileDescription} at ${path}: ${String(err)}`, { cause: err }); + } + try { + return { manifest: JSON.parse(raw) as PackageManifest, raw }; + } catch (err) { + throw new DependencySyncError(`Could not parse ${fileDescription} at ${path}: ${String(err)}`, { cause: err }); + } +} + +/** + * Sync the managed dependencies of a vended CDK project to the versions this CLI + * release was tested with. + * + * The vended asset package.json is the source of truth: every dependency named in it + * is managed and rewritten to its vended specifier (tilde for stable deps so patches + * float, exact for the L3 constructs); everything else in the user's file is untouched. + * Projects that predate pinning (caret ranges) are migrated automatically. If the + * project declares a managed dependency newer than this CLI expects, the project was + * updated by a newer CLI (or the user bumped it manually) and this throws + * CliVersionTooOldError (downgraded to a warning when `disabled` or + * `treatSkewAsWarning`). When anything changed — or node_modules is missing — + * `npm install` reconciles the edited package.json against the existing lockfile + * incrementally; node_modules and the lockfile are never deleted, so user-added deps + * keep their resolved versions. If the install fails, the pre-rewrite package.json is + * restored so a retry recomputes the same plan and re-attempts the install. + * + * `mode: 'check'` computes the same plan/warnings/notice without writing or + * installing, for preview flows that must not mutate the working tree. + * + * All user-facing wording is returned on the result (`notice`, `warnings`) — callers + * only display it. Throws CliVersionTooOldError | DependencySyncError; never exits. + */ +export async function syncManagedDependencies(options: SyncManagedDependenciesOptions): Promise { + const { + vendedPackageJsonPath, + projectDir, + disabled = false, + mode = 'apply', + treatSkewAsWarning = false, + installCommand = DEFAULT_CLI_INSTALL_COMMAND, + } = options; + const checkOnly = mode === 'check'; + const projectPackageJsonPath = join(projectDir, 'package.json'); + + const { manifest: vended } = await readManifest(vendedPackageJsonPath, 'the vended CDK package.json'); + // Keep the raw pre-rewrite text: it's restored verbatim if npm install fails, so a + // retry recomputes the same plan instead of seeing an already-matching manifest. + const { manifest: project, raw: projectRaw } = await readManifest( + projectPackageJsonPath, + "the project's CDK package.json" + ); + + const plan = computeSyncPlan(vended, project); + + const result: DependencySyncResult = { + optedOut: disabled, + checkOnly, + migratedFromCaret: plan.migratedFromCaret, + reinstalled: false, + skewWarning: false, + changes: plan.changes, + restored: plan.restored, + skipped: plan.skipped, + warnings: plan.skipped.map(formatSkippedWarning), + notice: null, + }; + + if (plan.skew.length > 0) { + if (!disabled && !treatSkewAsWarning && !checkOnly) { + throw new CliVersionTooOldError(formatCliUpgradeError(plan.skew, installCommand)); + } + result.skewWarning = true; + result.warnings.push(formatSkewWarning(plan.skew, installCommand)); + } + + if (disabled) { + return result; + } + + const hasPlannedChanges = plan.changes.length > 0 || plan.restored.length > 0; + + if (checkOnly) { + // Report what WOULD change without touching the working tree (future-tense wording). + result.notice = formatSyncNotice({ + migratedFromCaret: result.migratedFromCaret, + changes: result.changes, + restored: result.restored, + reinstalled: false, + applied: false, + }); + return result; + } + + if (hasPlannedChanges) { + // Mutate the parsed manifest in place so user key order and unknown fields survive. + for (const change of plan.changes) { + project[change.section]![change.name] = change.to; + } + for (const restoredDep of plan.restored) { + const section = (project[restoredDep.section] ??= {}); + section[restoredDep.name] = restoredDep.to; + } + + try { + await writeFile(projectPackageJsonPath, JSON.stringify(project, null, 2) + '\n', 'utf-8'); + } catch (err) { + throw new DependencySyncError(`Failed to update the CDK project's dependencies: ${String(err)}`, { cause: err }); + } + } + + // Install when the manifest changed OR node_modules is missing (e.g. the user deleted it, or + // `create --no-install` never populated it). npm >=7 reconciles the edited package.json + // against the existing lockfile incrementally, so we never delete node_modules or the + // lockfile: user-added deps keep their resolved versions and installs stay warm. + const needsInstall = hasPlannedChanges || !existsSync(join(projectDir, 'node_modules')); + if (!needsInstall) { + return result; + } + + const installResult = await runSubprocessCapture('npm', ['install'], { cwd: projectDir }); + if (installResult.code !== 0) { + // Restore the pre-rewrite package.json before failing. Without this, the retry would see a + // manifest that already matches the pins and an existing node_modules (a failed install + // doesn't remove it), skip the install, and deploy against the stale installed tree — the + // exact skew this sync exists to prevent. Restoring makes the retry recompute the same plan + // and re-attempt the install. Best-effort: a restore failure must not mask the install error. + if (hasPlannedChanges) { + await writeFile(projectPackageJsonPath, projectRaw, 'utf-8').catch(() => undefined); + } + throw new DependencySyncError( + `npm install failed after updating managed dependencies (exit ${String(installResult.code)}): ${installResult.stderr}` + ); + } + result.reinstalled = true; + + result.notice = formatSyncNotice({ + migratedFromCaret: result.migratedFromCaret, + changes: result.changes, + restored: result.restored, + reinstalled: result.reinstalled, + applied: true, + }); + return result; +} diff --git a/src/lib/dependency-management/types.ts b/src/lib/dependency-management/types.ts index fa4dacb27..bbe430350 100644 --- a/src/lib/dependency-management/types.ts +++ b/src/lib/dependency-management/types.ts @@ -32,7 +32,7 @@ export interface DependencySyncResult { /** True when this was a check-only run (`mode: 'check'`) — plan computed, nothing written or installed. */ checkOnly: boolean; /** True when the project predated pinning (caret ranges on managed deps were rewritten). */ - migrated: boolean; + migratedFromCaret: boolean; /** True when `npm install` was run to reconcile the installed tree (manifest changed, or node_modules was missing). */ reinstalled: boolean; /** True when a newer-than-CLI managed dep was found but downgraded to a warning (opted out or skew-as-warning). */ From c87ec90c4294dc9dd3cd3083e1b1fd8c32844701 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Sat, 18 Jul 2026 01:30:19 +0000 Subject: [PATCH 7/8] refactor: address review round 1 - add outcome discriminant to DependencySyncResult (synced/check-only/ opted-out/skipped/failure-suppressed/failed) + dep_sync_outcome telemetry - attach failedSyncResult on sync-step failures so telemetry records them - reset dep-sync state in startPreflight retries - accept v-prefixed semver versions (v1.2.3, ~v1.2.3) - replace NO_OP_SYNC_RESULT singleton with per-call factory - warn (not success) on suppressed teardown sync failure in headless progress - PreflightResult.dependencySync -> dependencySyncResult - export PackageManifest from barrel; drop redundant type re-exports - delete dead formatVersion; fix orphaned doc comment - retitle e2e opt-out test to what it actually proves; drop stale #1540 ref --- e2e-tests/dependency-sync.test.ts | 27 +++++++--- src/cli/commands/deploy/actions.ts | 16 ++++-- src/cli/commands/deploy/command.tsx | 6 ++- src/cli/commands/deploy/progress.ts | 6 ++- src/cli/logging/exec-logger.ts | 10 ++-- src/cli/operations/deploy/dependency-sync.ts | 50 +++++++++++-------- src/cli/operations/deploy/index.ts | 2 +- src/cli/telemetry/schemas/command-run.ts | 2 + src/cli/telemetry/schemas/common-shapes.ts | 2 + src/cli/tui/hooks/useCdkPreflight.ts | 19 ++++--- src/cli/tui/hooks/useDevDeploy.ts | 2 +- src/cli/tui/screens/deploy/useDeployFlow.ts | 12 ++--- .../__tests__/plan.test.ts | 7 +++ .../__tests__/semver.test.ts | 29 ++++++++--- .../__tests__/sync.test.ts | 3 ++ src/lib/dependency-management/index.ts | 2 + src/lib/dependency-management/semver.ts | 10 ++-- src/lib/dependency-management/sync.ts | 3 ++ src/lib/dependency-management/types.ts | 16 ++++++ 19 files changed, 156 insertions(+), 68 deletions(-) diff --git a/e2e-tests/dependency-sync.test.ts b/e2e-tests/dependency-sync.test.ts index d434d0183..975e45d6e 100644 --- a/e2e-tests/dependency-sync.test.ts +++ b/e2e-tests/dependency-sync.test.ts @@ -15,7 +15,7 @@ import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; /** - * E2E tests for the managed-dependency sync deploy preflight (#1540 / PR #1777). + * E2E tests for the managed-dependency sync deploy preflight. * * The unit tests in src/lib/dependency-management/__tests__/ cover the sync logic with the npm * subprocess mocked. These tests exercise the wiring through the real CLI binary — option @@ -36,6 +36,7 @@ const USER_DEP_SPEC = '^4.17.21'; /** Shape of the dependency-sync outcome inside the `deploy --json` envelope (DependencySyncResult). */ interface DepSyncJson { + outcome: 'synced' | 'check-only' | 'opted-out' | 'skipped' | 'failure-suppressed'; optedOut: boolean; checkOnly: boolean; migratedFromCaret: boolean; @@ -361,12 +362,19 @@ describe.sequential('e2e: dependency sync — newer-than-CLI skew fails fast wit }); // ───────────────────────────────────────────────────────────────────────────── -// Test 4 — the global opt-out downgrades skew to a warning -// Cheap: same shape as test 2. Uses an isolated AGENTCORE_CONFIG_DIR so the -// `agentcore config` write can't leak into the machine's real global config. +// Test 4 — the global opt-out config reaches the sync through the real CLI +// Cheap: same shape as test 2 (--dry-run, never deploys). Uses an isolated +// AGENTCORE_CONFIG_DIR so the `agentcore config` write can't leak into the +// machine's real global config. NOTE: because this is a --dry-run, check mode +// alone would already downgrade skew to a warning — the opt-out-specific signal +// here is `outcome: 'opted-out'` / `optedOut: true` in the JSON envelope. The +// behavioral downgrade (opt-out turning a would-be CliVersionTooOldError into a +// warning on a REAL deploy) is covered by the unit test "downgrades skew to a +// warning and touches nothing when disabled" in +// src/lib/dependency-management/__tests__/sync.test.ts. // ───────────────────────────────────────────────────────────────────────────── -describe.sequential('e2e: dependency sync — opt-out downgrades skew to a warning', () => { +describe.sequential('e2e: dependency sync — global opt-out config is plumbed through to the sync', () => { let testDir: string; let projectPath: string; let configDir: string; @@ -405,7 +413,7 @@ describe.sequential('e2e: dependency sync — opt-out downgrades skew to a warni }, 30000); it.skipIf(!canRun)( - 'deploy proceeds with a skew warning and leaves package.json unchanged', + 'sync reports opted-out, surfaces skew as a warning, and leaves package.json unchanged', async () => { expect(projectPath, 'Project should have been created').toBeTruthy(); @@ -419,8 +427,13 @@ describe.sequential('e2e: dependency sync — opt-out downgrades skew to a warni const sync = json.dependencySyncResult; expect(sync, 'Deploy result should carry dependencySyncResult').toBeDefined(); + // The opt-out-specific signal: the config write reached the sync through the real CLI. + // (This is a --dry-run, so check mode would surface skew as a warning even without the + // opt-out — outcome/optedOut is what proves the opt-out plumbing.) + expect(sync!.outcome, 'Opt-out should win over check mode in the outcome').toBe('opted-out'); expect(sync!.optedOut, 'Global opt-out should be reflected in the sync result').toBe(true); - expect(sync!.skewWarning, 'Skew should be downgraded to a warning when opted out').toBe(true); + // In this mode skew is surfaced as a warning, not an error. + expect(sync!.skewWarning, 'Skew should be surfaced as a warning in this mode').toBe(true); const warnings = sync!.warnings.join('\n'); expect(warnings).toContain(MANAGED_DEP); expect(warnings).toContain('newer than this CLI was tested with'); diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 996887345..3ea2f40d4 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -43,6 +43,7 @@ import { checkStackDeployability, ensureDefaultDeploymentTarget, ensureManagedDependencies, + failedSyncResult, getAllCredentials, hasIdentityApiProviders, hasIdentityOAuthProviders, @@ -78,7 +79,7 @@ export interface ValidatedDeployOptions { verbose?: boolean; plan?: boolean; diff?: boolean; - onProgress?: (step: string, status: 'start' | 'success' | 'error') => void; + onProgress?: (step: string, status: 'start' | 'success' | 'error' | 'warn') => void; onResourceEvent?: (message: string) => void; onDeployMessage?: (message: DeployMessage) => void; /** Emit a one-shot, user-facing notice to the terminal mid-deploy (e.g. the managed-memory heads-up @@ -190,7 +191,7 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { + const endStep = (status: 'success' | 'error' | 'warn', message?: string) => { logger.endStep(status, message); onProgress?.(currentStepName, status); }; @@ -294,6 +295,9 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise { // Progress callback for --progress mode const onProgress = options.progress - ? (step: string, status: 'start' | 'success' | 'error') => { + ? (step: string, status: 'start' | 'success' | 'error' | 'warn') => { if (spinner) { clearInterval(spinner); process.stdout.write('\r\x1b[K'); // Clear line @@ -106,6 +106,8 @@ async function executeDeploy(options: DeployOptions): Promise { }, 80); } else if (status === 'success') { console.log(`✓ ${step}`); + } else if (status === 'warn') { + console.log(`${ANSI.yellow}⚠ ${step}${ANSI.reset}`); } else { console.log(`✗ ${step}`); } diff --git a/src/cli/commands/deploy/progress.ts b/src/cli/commands/deploy/progress.ts index 09ec1cf4b..d37ec368c 100644 --- a/src/cli/commands/deploy/progress.ts +++ b/src/cli/commands/deploy/progress.ts @@ -8,7 +8,7 @@ import { handleDeploy } from './actions'; export const SPINNER_FRAMES = ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏']; export interface SpinnerProgress { - onProgress: (step: string, status: 'start' | 'success' | 'error') => void; + onProgress: (step: string, status: 'start' | 'success' | 'error' | 'warn') => void; cleanup: () => void; } @@ -23,7 +23,7 @@ export function createSpinnerProgress(): SpinnerProgress { } }; - const onProgress = (step: string, status: 'start' | 'success' | 'error') => { + const onProgress = (step: string, status: 'start' | 'success' | 'error' | 'warn') => { clearSpinner(); if (status === 'start') { @@ -35,6 +35,8 @@ export function createSpinnerProgress(): SpinnerProgress { }, 80); } else if (status === 'success') { console.log(`✓ ${step}`); + } else if (status === 'warn') { + console.log(`${ANSI.yellow}⚠ ${step}${ANSI.reset}`); } else { console.log(`✗ ${step}`); } diff --git a/src/cli/logging/exec-logger.ts b/src/cli/logging/exec-logger.ts index cc708e86b..77fdb62d8 100644 --- a/src/cli/logging/exec-logger.ts +++ b/src/cli/logging/exec-logger.ts @@ -139,19 +139,19 @@ ${separator} /** * Mark the end of the current step */ - endStep(status: 'success' | 'error', error?: string): void { + endStep(status: 'success' | 'error' | 'warn', error?: string): void { if (!this.currentStep) { return; } const duration = Date.now() - this.currentStep.startTime; - const statusText = status === 'success' ? 'SUCCESS' : 'FAILED'; + const statusText = status === 'success' ? 'SUCCESS' : status === 'warn' ? 'WARNING' : 'FAILED'; if (status === 'error') { this.lastFailedStep = this.currentStep.name; - if (error) { - this.appendLine(`[${this.formatTime()}] Error: ${error}`); - } + } + if ((status === 'error' || status === 'warn') && error) { + this.appendLine(`[${this.formatTime()}] ${status === 'error' ? 'Error' : 'Warning'}: ${error}`); } this.appendLine(`[${this.formatTime()}] Status: ${statusText}`); diff --git a/src/cli/operations/deploy/dependency-sync.ts b/src/cli/operations/deploy/dependency-sync.ts index 95ca403a0..02353101e 100644 --- a/src/cli/operations/deploy/dependency-sync.ts +++ b/src/cli/operations/deploy/dependency-sync.ts @@ -1,5 +1,5 @@ import { syncManagedDependencies } from '../../../lib/dependency-management'; -import type { DependencySyncResult } from '../../../lib/dependency-management'; +import type { DependencySyncOutcome, DependencySyncResult } from '../../../lib/dependency-management'; import { readGlobalConfigSync } from '../../../lib/schemas/io/global-config'; import type { LocalCdkProject } from '../../cdk/local-cdk-project'; import { getDistroConfig } from '../../constants'; @@ -23,12 +23,14 @@ export interface EnsureManagedDependenciesOptions { /** * A DependencySyncResult in which no dependency operation took effect: nothing was - * written, installed, or warned about. Base shape for the no-op states below. + * written or installed. `outcome` says why the sync was a no-op. Base shape for the + * no-op states below. */ -function noOpSyncResult(warnings: string[] = []): DependencySyncResult { +function noOpSyncResult(outcome: DependencySyncOutcome, warnings: string[] = []): DependencySyncResult { return { + outcome, optedOut: false, - checkOnly: true, + checkOnly: false, migratedFromCaret: false, reinstalled: false, skewWarning: false, @@ -40,8 +42,15 @@ function noOpSyncResult(warnings: string[] = []): DependencySyncResult { }; } -/** No-op result for when the sync is skipped entirely (AGENTCORE_SKIP_INSTALL). */ -const NO_OP_SYNC_RESULT: DependencySyncResult = noOpSyncResult(); +/** + * Minimal result attached to the failure path when the sync itself throws + * (CliVersionTooOldError, or DependencySyncError on a non-teardown deploy): nothing + * took effect, and dep_sync_* telemetry records that the sync was the failure. + * No warning line is added — the thrown error already carries the message. + */ +export function failedSyncResult(): DependencySyncResult { + return noOpSyncResult('failed'); +} /** * Warning-only result for a DependencySyncError caught on a teardown deploy. Teardown must @@ -51,26 +60,16 @@ const NO_OP_SYNC_RESULT: DependencySyncResult = noOpSyncResult(); * node_modules is genuinely unusable, the build step fails on its own. */ export function teardownSyncFailureResult(err: Error): DependencySyncResult { - return noOpSyncResult([`Dependency sync failed (continuing with teardown): ${err.message}`]); + return noOpSyncResult('failure-suppressed', [`Dependency sync failed (continuing with teardown): ${err.message}`]); } -/** - * Deploy-preflight entry point for managed dependency pinning: resolves the - * CLI's vended CDK package.json (source of truth), the global opt-out, and the - * distro-aware CLI install command, then runs the sync against the project's - * agentcore/cdk directory. A future `agentcore build` command should call this same - * function. Skipped entirely when AGENTCORE_SKIP_INSTALL is set (same escape hatch - * as create --no-install and the language setup steps). - * - * Throws CliVersionTooOldError when the project was updated by a newer CLI (unless - * `treatSkewAsWarning` or opted out), and DependencySyncError on rewrite/install failure. - */ /** * Map a dependency sync result to its dep_sync_* telemetry attrs. * Single source for both the CLI command (command.tsx) and the TUI flow (useDeployFlow). */ export function toDepSyncAttrs(sync: DependencySyncResult) { return { + dep_sync_outcome: sync.outcome, dep_sync_changed_count: sync.changes.length + sync.restored.length, dep_sync_migrated: sync.migratedFromCaret, dep_sync_opted_out: sync.optedOut, @@ -79,12 +78,23 @@ export function toDepSyncAttrs(sync: DependencySyncResult) { }; } +/** + * Deploy-preflight entry point for managed dependency pinning: resolves the + * CLI's vended CDK package.json (source of truth), the global opt-out, and the + * distro-aware CLI install command, then runs the sync against the project's + * agentcore/cdk directory. A future `agentcore build` command should call this same + * function. Skipped entirely when AGENTCORE_SKIP_INSTALL is set (same escape hatch + * as create --no-install and the language setup steps). + * + * Throws CliVersionTooOldError when the project was updated by a newer CLI (unless + * `treatSkewAsWarning` or opted out), and DependencySyncError on rewrite/install failure. + */ export async function ensureManagedDependencies( cdkProject: LocalCdkProject, options: EnsureManagedDependenciesOptions = {} ): Promise { if (process.env.AGENTCORE_SKIP_INSTALL) { - return NO_OP_SYNC_RESULT; + return noOpSyncResult('skipped'); } const disabled = readGlobalConfigSync().disableDependencyManagement === true; return syncManagedDependencies({ @@ -96,5 +106,3 @@ export async function ensureManagedDependencies( installCommand: getDistroConfig().installCommand, }); } - -export type { DependencySyncResult }; diff --git a/src/cli/operations/deploy/index.ts b/src/cli/operations/deploy/index.ts index 0bba391cb..7e0d53e4a 100644 --- a/src/cli/operations/deploy/index.ts +++ b/src/cli/operations/deploy/index.ts @@ -71,10 +71,10 @@ export { backfillContainerVpcIds, type BackfillVpcIdResult } from './backfill-vp // Managed dependency pinning — shared by CLI deploy + TUI preflight export { ensureManagedDependencies, + failedSyncResult, teardownSyncFailureResult, toDepSyncAttrs, SYNC_CDK_DEPENDENCIES_STEP, - type DependencySyncResult, type EnsureManagedDependenciesOptions, } from './dependency-sync'; diff --git a/src/cli/telemetry/schemas/command-run.ts b/src/cli/telemetry/schemas/command-run.ts index 42094b092..0fca9cc3c 100644 --- a/src/cli/telemetry/schemas/command-run.ts +++ b/src/cli/telemetry/schemas/command-run.ts @@ -10,6 +10,7 @@ import { BuildType, Count, CredentialType, + DepSyncOutcome, DeployModeSchema, DevAction, EvaluatorLevel, @@ -119,6 +120,7 @@ const DeployAttrs = safeSchema({ policy_engine_count: Count, policy_count: Count, deploy_mode: DeployModeSchema, + dep_sync_outcome: DepSyncOutcome.optional(), dep_sync_changed_count: Count.optional(), dep_sync_migrated: z.boolean().optional(), dep_sync_opted_out: z.boolean().optional(), diff --git a/src/cli/telemetry/schemas/common-shapes.ts b/src/cli/telemetry/schemas/common-shapes.ts index 255268bee..a319cf570 100644 --- a/src/cli/telemetry/schemas/common-shapes.ts +++ b/src/cli/telemetry/schemas/common-shapes.ts @@ -35,6 +35,8 @@ export const AuthType = z.enum(['sigv4', 'bearer_token']); export const AuthorizerType = z.enum(['aws_iam', 'custom_jwt', 'none']); export const BuildType = z.enum(['codezip', 'container']); export const CredentialType = z.enum(['api-key', 'oauth']); +// Mirrors DependencySyncOutcome in src/lib/dependency-management/types.ts. +export const DepSyncOutcome = z.enum(['synced', 'check-only', 'opted-out', 'skipped', 'failure-suppressed', 'failed']); export const SkillSourceType = z.enum(['path', 's3', 'git', 'aws_skills']); export const EvaluatorType = z.enum(['llm-as-a-judge', 'code-based']); export const ExitReason = z.enum(['success', 'failure']); diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index 7a124896c..f78044db3 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -17,6 +17,7 @@ import { checkDependencyVersions, checkStackDeployability, ensureManagedDependencies, + failedSyncResult, formatError, getAllCredentials, hasIdentityApiProviders, @@ -173,7 +174,7 @@ export interface PreflightResult { /** KMS key ARN used for identity token vault encryption */ identityKmsKeyArn?: string; /** Result of the managed dependency sync — notice/warnings for display, attrs for telemetry */ - dependencySync: DependencySyncResult | null; + dependencySyncResult: DependencySyncResult | null; /** Credential ARNs (API key + OAuth) from pre-deploy setup */ allCredentials: Record; startPreflight: () => Promise; @@ -240,7 +241,7 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { Record >({}); const [teardownConfirmed, setTeardownConfirmed] = useState(false); - const [dependencySync, setDependencySync] = useState(null); + const [dependencySyncResult, setDependencySyncResult] = useState(null); const lastErrorRef = useRef(undefined); // Guard against concurrent runs (React StrictMode, re-renders, etc.) @@ -297,6 +298,9 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { setBootstrapContext(null); setHasTokenExpiredError(false); // Reset token expired state when retrying setHasCredentialsError(false); // Reset credentials error state when retrying + // Reset the previous run's dep-sync result so a retry doesn't render stale + // notices/warnings or attach stale dep_sync_* attrs to the new run's telemetry. + setDependencySyncResult(null); setPhase('running'); }, [disposeWrapper, restoreRegionEnv]); @@ -519,23 +523,26 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { if (syncResult.notice) { logger.log(syncResult.notice); } - setDependencySync(syncResult); + setDependencySyncResult(syncResult); logger.endStep('success'); updateStep(STEP_SYNC_DEPS, { status: 'success' }); } catch (err) { if (preflightContext.isTeardownDeploy && err instanceof DependencySyncError) { // Surface through the same warning channels as a downgraded skew: the deploy-flow - // screens render dependencySync.warnings, and the step shows the warning inline. + // screens render dependencySyncResult.warnings, and the step shows the warning inline. const downgraded = teardownSyncFailureResult(err); const warning = downgraded.warnings[0]!; logger.log(warning, 'warn'); - setDependencySync(downgraded); + setDependencySyncResult(downgraded); logger.endStep('success'); updateStep(STEP_SYNC_DEPS, { status: 'warn', warn: warning }); } else { const errorMsg = formatError(err); logger.endStep('error', errorMsg); updateStep(STEP_SYNC_DEPS, { status: 'error', error: getErrorMessage(err) }); + // The sync itself is the failure: attach a minimal 'failed' result so + // useDeployFlow's failure telemetry still carries dep_sync_* attrs. + setDependencySyncResult(failedSyncResult()); failPreflight(err); return; } @@ -1112,7 +1119,7 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { lastError: lastErrorRef.current, missingCredentials, identityKmsKeyArn, - dependencySync, + dependencySyncResult, allCredentials, startPreflight, confirmTeardown, diff --git a/src/cli/tui/hooks/useDevDeploy.ts b/src/cli/tui/hooks/useDevDeploy.ts index ad42f5d44..4327311e9 100644 --- a/src/cli/tui/hooks/useDevDeploy.ts +++ b/src/cli/tui/hooks/useDevDeploy.ts @@ -40,7 +40,7 @@ export function useDevDeploy({ skip, ready = true }: UseDevDeployOptions = {}): const [dependencySyncWarnings, setDependencySyncWarnings] = useState([]); const hasStarted = useRef(false); - const onProgress = useCallback((stepName: string, status: 'start' | 'success' | 'error') => { + const onProgress = useCallback((stepName: string, status: 'start' | 'success' | 'error' | 'warn') => { setSteps(prev => { if (status === 'start') { return [...prev, { label: stepName, status: 'running' }]; diff --git a/src/cli/tui/screens/deploy/useDeployFlow.ts b/src/cli/tui/screens/deploy/useDeployFlow.ts index 7bf07645d..72586ff6b 100644 --- a/src/cli/tui/screens/deploy/useDeployFlow.ts +++ b/src/cli/tui/screens/deploy/useDeployFlow.ts @@ -757,7 +757,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState const error = preflight.lastError ?? new Error('Preflight failed'); const attrs = withDepSyncAttrs( context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }, - preflight.dependencySync + preflight.dependencySyncResult ); withCommandRunTelemetry('deploy', attrs, () => ({ success: false as const, error })).catch(() => { /* telemetry is best-effort */ @@ -770,7 +770,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState const attrs = withDepSyncAttrs( context ? computeDeployAttrs(context.projectSpec, 'deploy') : { ...DEFAULT_DEPLOY_ATTRS }, - preflight.dependencySync + preflight.dependencySyncResult ); const run = async (): Promise<{ success: true } | { success: false; error: Error }> => { @@ -1046,7 +1046,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState context ? computeDeployAttrs(context.projectSpec, 'diff') : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }, - preflight.dependencySync + preflight.dependencySyncResult ); withCommandRunTelemetry('deploy', attrs, () => ({ success: false as const, error })).catch(() => { /* telemetry is best-effort */ @@ -1061,7 +1061,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState context ? computeDeployAttrs(context.projectSpec, 'diff') : { ...DEFAULT_DEPLOY_ATTRS, deploy_mode: 'diff' as const }, - preflight.dependencySync + preflight.dependencySyncResult ); const run = async (): Promise<{ success: true } | { success: false; error: Error }> => { @@ -1259,8 +1259,8 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState numStacksWithChanges, deployNotes, managedMemoryNotice, - dependencySyncNotice: preflight.dependencySync?.notice ?? null, - dependencySyncWarnings: preflight.dependencySync?.warnings ?? [], + dependencySyncNotice: preflight.dependencySyncResult?.notice ?? null, + dependencySyncWarnings: preflight.dependencySyncResult?.warnings ?? [], postDeployWarnings, postDeployHasError, isDiffLoading, diff --git a/src/lib/dependency-management/__tests__/plan.test.ts b/src/lib/dependency-management/__tests__/plan.test.ts index be2aa5237..e683fa250 100644 --- a/src/lib/dependency-management/__tests__/plan.test.ts +++ b/src/lib/dependency-management/__tests__/plan.test.ts @@ -102,6 +102,13 @@ describe('computeSyncPlan', () => { ]); }); + it('syncs v-prefixed specifiers instead of skipping them as non-semver', () => { + const plan = computeSyncPlan(VENDED, project({ dependencies: { ...VENDED.dependencies, constructs: 'v10.7.0' } })); + expect(plan.skipped).toEqual([]); + expect(plan.skew).toEqual([]); + expect(plan.changes).toEqual([{ name: 'constructs', section: 'dependencies', from: 'v10.7.0', to: '~10.7.0' }]); + }); + it('never touches user-added dependencies', () => { const plan = computeSyncPlan( VENDED, diff --git a/src/lib/dependency-management/__tests__/semver.test.ts b/src/lib/dependency-management/__tests__/semver.test.ts index 9450055d5..630ca0411 100644 --- a/src/lib/dependency-management/__tests__/semver.test.ts +++ b/src/lib/dependency-management/__tests__/semver.test.ts @@ -1,4 +1,4 @@ -import { compareVersions, formatVersion, parseSpecifier, parseVersion } from '../semver'; +import { compareVersions, parseSpecifier, parseVersion } from '../semver'; import { describe, expect, it } from 'vitest'; describe('parseVersion', () => { @@ -14,6 +14,12 @@ describe('parseVersion', () => { expect(parseVersion('1.2.3+build.5')).toEqual({ major: 1, minor: 2, patch: 3, prerelease: [] }); }); + it('accepts a single leading v, like npm', () => { + expect(parseVersion('v1.2.3')).toEqual({ major: 1, minor: 2, patch: 3, prerelease: [] }); + expect(parseVersion('V10.7.0')).toEqual({ major: 10, minor: 7, patch: 0, prerelease: [] }); + expect(parseVersion('vv1.2.3')).toBeNull(); + }); + it('rejects non-versions', () => { expect(parseVersion('latest')).toBeNull(); expect(parseVersion('1.2')).toBeNull(); @@ -56,6 +62,20 @@ describe('parseSpecifier', () => { expect(parseSpecifier('=1.2.3').kind).toBe('exact'); }); + it('accepts v-prefixed versions, bare and ranged, like npm', () => { + expect(parseSpecifier('v1.2.3')).toEqual({ + kind: 'exact', + version: { major: 1, minor: 2, patch: 3, prerelease: [] }, + raw: 'v1.2.3', + }); + expect(parseSpecifier('~v1.2.3')).toEqual({ + kind: 'tilde', + version: { major: 1, minor: 2, patch: 3, prerelease: [] }, + raw: '~v1.2.3', + }); + expect(parseSpecifier('^v10.7.0').kind).toBe('caret'); + }); + it('marks non-semver specifiers unsupported', () => { for (const raw of [ 'file:bundled-agentcore-cdk.tgz', @@ -71,10 +91,3 @@ describe('parseSpecifier', () => { } }); }); - -describe('formatVersion', () => { - it('round-trips release and prerelease versions', () => { - expect(formatVersion(parseVersion('2.261.0')!)).toBe('2.261.0'); - expect(formatVersion(parseVersion('0.1.0-alpha.45')!)).toBe('0.1.0-alpha.45'); - }); -}); diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts index c535a0848..c81e2f592 100644 --- a/src/lib/dependency-management/__tests__/sync.test.ts +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -54,6 +54,7 @@ describe('syncManagedDependencies', () => { it('is a no-op when the project already matches (no write, no reinstall, no notice)', async () => { writeProject({ dependencies: { ...VENDED.dependencies }, devDependencies: { ...VENDED.devDependencies } }); const result = await run(); + expect(result.outcome).toBe('synced'); expect(result.changes).toEqual([]); expect(result.reinstalled).toBe(false); expect(result.notice).toBeNull(); @@ -143,6 +144,7 @@ describe('syncManagedDependencies', () => { }; writeProject(manifest); const result = await run({ disabled: true }); + expect(result.outcome).toBe('opted-out'); expect(result.optedOut).toBe(true); expect(result.skewWarning).toBe(true); expect(result.warnings.some(w => w.includes('newer than this CLI was tested with'))).toBe(true); @@ -171,6 +173,7 @@ describe('syncManagedDependencies', () => { }; writeProject(manifest); const result = await run({ mode: 'check' }); + expect(result.outcome).toBe('check-only'); expect(result.checkOnly).toBe(true); expect(result.changes).toHaveLength(2); expect(result.reinstalled).toBe(false); diff --git a/src/lib/dependency-management/index.ts b/src/lib/dependency-management/index.ts index 2165227b5..85a40573c 100644 --- a/src/lib/dependency-management/index.ts +++ b/src/lib/dependency-management/index.ts @@ -3,7 +3,9 @@ export { computeSyncPlan } from './plan'; export type { SkewFinding, SyncPlan } from './plan'; export type { DependencyChange, + DependencySyncOutcome, DependencySyncResult, + PackageManifest, RestoredDependency, SkippedDependency, SyncManagedDependenciesOptions, diff --git a/src/lib/dependency-management/semver.ts b/src/lib/dependency-management/semver.ts index ebd7bc33c..8ce141a02 100644 --- a/src/lib/dependency-management/semver.ts +++ b/src/lib/dependency-management/semver.ts @@ -24,7 +24,10 @@ export type ParsedSpecifier = const VERSION_RE = /^(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-.]+)?$/; export function parseVersion(version: string): ParsedVersion | null { - const match = VERSION_RE.exec(version.trim()); + // npm accepts a single leading 'v' on version strings (v1.2.3, ~v1.2.3); stripping it + // here covers both bare versions and the version part of a range specifier. + const normalized = version.trim().replace(/^[vV]/, ''); + const match = VERSION_RE.exec(normalized); if (!match) return null; const prerelease = match[4] ? match[4].split('.').map(id => (/^\d+$/.test(id) ? Number(id) : id)) : []; return { @@ -89,8 +92,3 @@ export function parseSpecifier(raw: string): ParsedSpecifier { if (!version) return { kind: 'unsupported', raw }; return { kind, version, raw }; } - -export function formatVersion(v: ParsedVersion): string { - const base = `${v.major}.${v.minor}.${v.patch}`; - return v.prerelease.length > 0 ? `${base}-${v.prerelease.join('.')}` : base; -} diff --git a/src/lib/dependency-management/sync.ts b/src/lib/dependency-management/sync.ts index 941ac36c9..b9285f210 100644 --- a/src/lib/dependency-management/sync.ts +++ b/src/lib/dependency-management/sync.ts @@ -75,7 +75,10 @@ export async function syncManagedDependencies(options: SyncManagedDependenciesOp const plan = computeSyncPlan(vended, project); + // `disabled` wins over check mode: an opted-out sync never writes regardless of mode. + const outcome = disabled ? 'opted-out' : checkOnly ? 'check-only' : 'synced'; const result: DependencySyncResult = { + outcome, optedOut: disabled, checkOnly, migratedFromCaret: plan.migratedFromCaret, diff --git a/src/lib/dependency-management/types.ts b/src/lib/dependency-management/types.ts index bbe430350..c75e39614 100644 --- a/src/lib/dependency-management/types.ts +++ b/src/lib/dependency-management/types.ts @@ -26,7 +26,23 @@ export interface SkippedDependency { reason: string; } +/** + * How the sync concluded: + * - 'synced': the sync ran in apply mode (rewrites/installs happened as needed). + * - 'check-only': `mode: 'check'` — plan computed, nothing written or installed. + * - 'opted-out': dependency management disabled via global config — nothing written. + * - 'skipped': the CLI layer skipped the sync entirely (AGENTCORE_SKIP_INSTALL). + * - 'failure-suppressed': the sync threw on a teardown deploy and the failure was + * downgraded to a warning so the teardown could proceed. + * - 'failed': the sync threw and the deploy failed with it (CliVersionTooOldError, or + * DependencySyncError on a non-teardown deploy). Attached by the CLI layer's failure + * path purely so dep_sync_* telemetry records that the sync itself was the failure. + */ +export type DependencySyncOutcome = 'synced' | 'check-only' | 'opted-out' | 'skipped' | 'failure-suppressed' | 'failed'; + export interface DependencySyncResult { + /** How the sync concluded. Discriminant for the boolean flags below. */ + outcome: DependencySyncOutcome; /** True when dependency management is disabled via global config — nothing was written. */ optedOut: boolean; /** True when this was a check-only run (`mode: 'check'`) — plan computed, nothing written or installed. */ From dc1aef32517deffcd00a410e718d09b91b5fa077 Mon Sep 17 00:00:00 2001 From: Jesse Turner Date: Sat, 18 Jul 2026 01:46:44 +0000 Subject: [PATCH 8/8] polish: address final review nits - TUI logs warn (not success) for suppressed teardown sync failure - dedupe suppressed-failure warning line in deploy log - e2e: add 'failed' outcome to DepSyncJson union + assert it in skew test - add unit tests for CLI-layer sync factories, skip branch, toDepSyncAttrs, and disabled-wins-over-check outcome precedence - fix outcome doc overclaim; rename endStep error param to message --- e2e-tests/dependency-sync.test.ts | 4 +- src/cli/commands/deploy/actions.ts | 5 +- src/cli/logging/exec-logger.ts | 9 +- .../deploy/__tests__/dependency-sync.test.ts | 107 ++++++++++++++++++ src/cli/tui/hooks/useCdkPreflight.ts | 3 +- .../__tests__/sync.test.ts | 16 +++ src/lib/dependency-management/types.ts | 5 +- 7 files changed, 140 insertions(+), 9 deletions(-) create mode 100644 src/cli/operations/deploy/__tests__/dependency-sync.test.ts diff --git a/e2e-tests/dependency-sync.test.ts b/e2e-tests/dependency-sync.test.ts index 975e45d6e..3d30a227c 100644 --- a/e2e-tests/dependency-sync.test.ts +++ b/e2e-tests/dependency-sync.test.ts @@ -36,7 +36,7 @@ const USER_DEP_SPEC = '^4.17.21'; /** Shape of the dependency-sync outcome inside the `deploy --json` envelope (DependencySyncResult). */ interface DepSyncJson { - outcome: 'synced' | 'check-only' | 'opted-out' | 'skipped' | 'failure-suppressed'; + outcome: 'synced' | 'check-only' | 'opted-out' | 'skipped' | 'failure-suppressed' | 'failed'; optedOut: boolean; checkOnly: boolean; migratedFromCaret: boolean; @@ -352,6 +352,8 @@ describe.sequential('e2e: dependency sync — newer-than-CLI skew fails fast wit expect(json.error).toContain('requires a newer version of the AgentCore CLI'); expect(json.error).toContain(MANAGED_DEP); expect(json.errorName).toBe('CliVersionTooOldError'); + // The failure envelope still carries the sync outcome (dep_sync_* telemetry rides on it). + expect(json.dependencySyncResult?.outcome).toBe('failed'); // Skew is detected before anything is written — the manifest must be untouched. const { raw } = await readCdkManifest(projectPath); diff --git a/src/cli/commands/deploy/actions.ts b/src/cli/commands/deploy/actions.ts index 3ea2f40d4..168790440 100644 --- a/src/cli/commands/deploy/actions.ts +++ b/src/cli/commands/deploy/actions.ts @@ -311,9 +311,10 @@ export async function handleDeploy(options: ValidatedDeployOptions): Promise ({ + mockSyncManagedDependencies: vi.fn(), +})); + +vi.mock('../../../../lib/dependency-management', () => ({ + syncManagedDependencies: mockSyncManagedDependencies, +})); + +vi.mock('../../../../lib/schemas/io/global-config', () => ({ + readGlobalConfigSync: () => ({}), +})); + +/** Every field of the no-op base shape shared by failedSyncResult / teardownSyncFailureResult. */ +function expectNoOpShape(result: DependencySyncResult): void { + expect(result.optedOut).toBe(false); + expect(result.checkOnly).toBe(false); + expect(result.migratedFromCaret).toBe(false); + expect(result.reinstalled).toBe(false); + expect(result.skewWarning).toBe(false); + expect(result.changes).toEqual([]); + expect(result.restored).toEqual([]); + expect(result.skipped).toEqual([]); + expect(result.notice).toBeNull(); +} + +describe('failedSyncResult', () => { + it('is a no-op result with outcome "failed" and no warning line (the thrown error carries the message)', () => { + const result = failedSyncResult(); + expect(result.outcome).toBe('failed'); + expect(result.warnings).toEqual([]); + expectNoOpShape(result); + }); +}); + +describe('teardownSyncFailureResult', () => { + it('is a no-op result with outcome "failure-suppressed" and the failure surfaced as a warning', () => { + const result = teardownSyncFailureResult(new Error('npm install failed (exit 1): E404')); + expect(result.outcome).toBe('failure-suppressed'); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain('Dependency sync failed (continuing with teardown)'); + expect(result.warnings[0]).toContain('npm install failed (exit 1): E404'); + expectNoOpShape(result); + }); +}); + +describe('ensureManagedDependencies', () => { + const origSkipInstall = process.env.AGENTCORE_SKIP_INSTALL; + + afterEach(() => { + vi.clearAllMocks(); + if (origSkipInstall !== undefined) process.env.AGENTCORE_SKIP_INSTALL = origSkipInstall; + else delete process.env.AGENTCORE_SKIP_INSTALL; + }); + + it('skips the sync entirely when AGENTCORE_SKIP_INSTALL is set', async () => { + process.env.AGENTCORE_SKIP_INSTALL = '1'; + + const result = await ensureManagedDependencies({ projectDir: '/project/agentcore/cdk' } as LocalCdkProject); + + expect(result.outcome).toBe('skipped'); + expect(result.warnings).toEqual([]); + expectNoOpShape(result); + expect(mockSyncManagedDependencies).not.toHaveBeenCalled(); + }); +}); + +describe('toDepSyncAttrs', () => { + it('maps a sync result to dep_sync_* telemetry attrs, including the outcome', () => { + const result: DependencySyncResult = { + outcome: 'synced', + optedOut: false, + checkOnly: false, + migratedFromCaret: true, + reinstalled: true, + skewWarning: true, + changes: [{ name: 'aws-cdk-lib', section: 'dependencies', from: '^2.248.0', to: '~2.261.0' }], + restored: [{ name: 'constructs', section: 'dependencies', to: '~10.4.2' }], + skipped: [], + warnings: ['skew warning'], + notice: 'updated', + }; + + expect(toDepSyncAttrs(result)).toEqual({ + dep_sync_outcome: 'synced', + dep_sync_changed_count: 2, + dep_sync_migrated: true, + dep_sync_opted_out: false, + dep_sync_skew_warning: true, + dep_sync_reinstalled: true, + }); + }); + + it('maps every no-op outcome through dep_sync_outcome', () => { + expect(toDepSyncAttrs(failedSyncResult()).dep_sync_outcome).toBe('failed'); + expect(toDepSyncAttrs(teardownSyncFailureResult(new Error('boom'))).dep_sync_outcome).toBe('failure-suppressed'); + }); +}); diff --git a/src/cli/tui/hooks/useCdkPreflight.ts b/src/cli/tui/hooks/useCdkPreflight.ts index f78044db3..68dcc4874 100644 --- a/src/cli/tui/hooks/useCdkPreflight.ts +++ b/src/cli/tui/hooks/useCdkPreflight.ts @@ -534,7 +534,8 @@ export function useCdkPreflight(options: PreflightOptions): PreflightResult { const warning = downgraded.warnings[0]!; logger.log(warning, 'warn'); setDependencySyncResult(downgraded); - logger.endStep('success'); + // No message: the logger.log above already wrote the warning line. + logger.endStep('warn'); updateStep(STEP_SYNC_DEPS, { status: 'warn', warn: warning }); } else { const errorMsg = formatError(err); diff --git a/src/lib/dependency-management/__tests__/sync.test.ts b/src/lib/dependency-management/__tests__/sync.test.ts index c81e2f592..9b59195b2 100644 --- a/src/lib/dependency-management/__tests__/sync.test.ts +++ b/src/lib/dependency-management/__tests__/sync.test.ts @@ -183,6 +183,22 @@ describe('syncManagedDependencies', () => { expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); }); + it('disabled wins over check mode in the outcome (opted-out, with checkOnly preserved)', async () => { + const manifest = { + dependencies: { '@aws/agentcore-cdk': '^0.1.0-alpha.19', 'aws-cdk-lib': '~2.261.0' }, + devDependencies: { typescript: '~5.9.3' }, + }; + writeProject(manifest); + const result = await run({ disabled: true, mode: 'check' }); + expect(result.outcome).toBe('opted-out'); + expect(result.optedOut).toBe(true); + // The overlapping detail survives: the run was also check-only. + expect(result.checkOnly).toBe(true); + expect(result.notice).toBeNull(); + expect(readProject()).toEqual(manifest); + expect(mockRunSubprocessCapture).not.toHaveBeenCalled(); + }); + it('check mode reports skew as a warning instead of throwing', async () => { const manifest = { dependencies: { ...VENDED.dependencies, 'aws-cdk-lib': '~2.300.0' }, devDependencies: {} }; writeProject(manifest); diff --git a/src/lib/dependency-management/types.ts b/src/lib/dependency-management/types.ts index c75e39614..8355d18f2 100644 --- a/src/lib/dependency-management/types.ts +++ b/src/lib/dependency-management/types.ts @@ -41,7 +41,10 @@ export interface SkippedDependency { export type DependencySyncOutcome = 'synced' | 'check-only' | 'opted-out' | 'skipped' | 'failure-suppressed' | 'failed'; export interface DependencySyncResult { - /** How the sync concluded. Discriminant for the boolean flags below. */ + /** + * Primary classification of the sync run. The booleans below preserve overlapping detail + * (e.g. an opted-out run in check mode also has `checkOnly: true`). + */ outcome: DependencySyncOutcome; /** True when dependency management is disabled via global config — nothing was written. */ optedOut: boolean;