From d38493be60f7df13f9aa7610f556a4fc2634db69 Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Mon, 6 Oct 2025 10:38:41 +0200 Subject: [PATCH 1/2] refactor(toolkit-lib): standardize confirmation requests to use ConfirmationRequest interface - Convert CDK_TOOLKIT_I8910 from DataRequest to ConfirmationRequest for refactor confirmation - Replace promptly.confirm usage with standardized IO message system in destroy and deploy flows - Update askUserConfirmation to use ActionLessRequest pattern - Remove deprecated markTesting function and TESTING variable - Update tests to use requestSpy instead of promptly mocks --- .../lib/api/io/private/messages.ts | 3 +- .../toolkit-lib/lib/toolkit/toolkit.ts | 7 +- .../toolkit-lib/test/actions/refactor.test.ts | 2 +- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 71 ++++++++----------- packages/aws-cdk/test/cli/cdk-toolkit.test.ts | 13 ++-- 5 files changed, 40 insertions(+), 56 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts index 36aecaa74..82435c09a 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts @@ -22,7 +22,6 @@ import type { AssemblyData, ConfirmationRequest, ContextProviderMessageSource, - DataRequest, Duration, ErrorPayload, SingleStack, @@ -391,7 +390,7 @@ export const IO = { interface: 'RefactorResult', }), - CDK_TOOLKIT_I8910: make.question({ + CDK_TOOLKIT_I8910: make.confirm({ code: 'CDK_TOOLKIT_I8910', description: 'Confirm refactor', interface: 'ConfirmationRequest', diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 27bce6055..36370da15 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -1190,10 +1190,9 @@ export class Toolkit extends CloudAssemblySourceBuilder { } const question = 'Do you wish to refactor these resources?'; - const response = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I8910.req(question, { - responseDescription: '[Y]es/[n]o', - }, 'y')); - return ['y', 'yes'].includes(response.toLowerCase()); + return ioHelper.requestResponse(IO.CDK_TOOLKIT_I8910.req(question, { + motivation: 'User input is needed', + })); } function formatError(error: any): string { diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts index ce5b21600..30d8fc6e2 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts @@ -854,7 +854,7 @@ describe('refactor execution', () => { expect.objectContaining({ action: 'refactor', code: 'CDK_TOOLKIT_I8910', - defaultResponse: 'y', + defaultResponse: true, level: 'info', message: 'Do you wish to refactor these resources?', }), diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 7d8edc5c5..ce4544baa 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -2,18 +2,17 @@ import * as path from 'path'; import { format } from 'util'; import { RequireApproval } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; -import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib'; +import type { ConfirmationRequest, DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib'; import { PermissionChangeType, Toolkit, ToolkitError } from '@aws-cdk/toolkit-lib'; import * as chalk from 'chalk'; import * as chokidar from 'chokidar'; import * as fs from 'fs-extra'; -import * as promptly from 'promptly'; import * as uuid from 'uuid'; import { CliIoHost } from './io-host'; import type { Configuration } from './user-configuration'; import { PROJECT_CONFIG } from './user-configuration'; -import type { IoHelper } from '../../lib/api-private'; -import { asIoHelper, cfnApi, tagsForStack } from '../../lib/api-private'; +import type { ActionLessRequest, IoHelper } from '../../lib/api-private'; +import { asIoHelper, cfnApi, IO, tagsForStack } from '../../lib/api-private'; import type { AssetBuildNode, AssetPublishNode, Concurrency, StackNode, WorkGraph } from '../api'; import { CloudWatchLogEventMonitor, @@ -74,12 +73,6 @@ import type { ErrorDetails } from './telemetry/schema'; // eslint-disable-next-line @typescript-eslint/no-require-imports,@typescript-eslint/consistent-type-imports const pLimit: typeof import('p-limit') = require('p-limit'); -let TESTING = false; - -export function markTesting() { - TESTING = true; -} - export interface CdkToolkitProps { /** * The Cloud Executable @@ -495,12 +488,16 @@ export class CdkToolkit { }); const securityDiff = formatter.formatSecurityDiff(); if (requiresApproval(requireApproval, securityDiff.permissionChangeType)) { + const motivation = '"--require-approval" is enabled and stack includes security-sensitive updates'; await this.ioHost.asIoHelper().defaults.info(securityDiff.formattedDiff); await askUserConfirmation( this.ioHost, - concurrency, - '"--require-approval" is enabled and stack includes security-sensitive updates', - 'Do you wish to deploy these changes', + IO.CDK_TOOLKIT_I5060.req(`${motivation}: 'Do you wish to deploy these changes'`, { + motivation, + concurrency, + permissionChangeType: securityDiff.permissionChangeType, + templateDiffs: formatter.diffs, + }), ); } } @@ -578,9 +575,10 @@ export class CdkToolkit { } else { await askUserConfirmation( this.ioHost, - concurrency, - motivation, - `${motivation}. Roll back first and then proceed with deployment`, + IO.CDK_TOOLKIT_I5050.req(`${motivation}. Roll back first and then proceed with deployment`, { + motivation, + concurrency, + }), ); } @@ -604,9 +602,10 @@ export class CdkToolkit { } else { await askUserConfirmation( this.ioHost, - concurrency, - motivation, - `${motivation}. Perform a regular deployment`, + IO.CDK_TOOLKIT_I5050.req(`${motivation}. Perform a regular deployment`, { + concurrency, + motivation, + }), ); } @@ -970,33 +969,33 @@ export class CdkToolkit { } public async destroy(options: DestroyOptions) { - let stacks = await this.selectStacksForDestroy(options.selector, options.exclusively); + const ioHelper = this.ioHost.asIoHelper(); // The stacks will have been ordered for deployment, so reverse them for deletion. - stacks = stacks.reversed(); + const stacks = (await this.selectStacksForDestroy(options.selector, options.exclusively)).reversed(); if (!options.force) { - // eslint-disable-next-line @stylistic/max-len - const confirmed = await promptly.confirm( - `Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))} (y/n)?`, - ); + const motivation = 'Destroying stacks is an irreversible action'; + const question = `Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))}`; + const confirmed = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I7010.req(question, { motivation })); if (!confirmed) { + await ioHelper.notify(IO.CDK_TOOLKIT_E7010.msg('Aborted by user')); return; } } const action = options.fromDeploy ? 'deploy' : 'destroy'; for (const [index, stack] of stacks.stackArtifacts.entries()) { - await this.ioHost.asIoHelper().defaults.info(chalk.green('%s: destroying... [%s/%s]'), chalk.blue(stack.displayName), index + 1, stacks.stackCount); + await ioHelper.defaults.info(chalk.green('%s: destroying... [%s/%s]'), chalk.blue(stack.displayName), index + 1, stacks.stackCount); try { await this.props.deployments.destroyStack({ stack, deployName: stack.stackName, roleArn: options.roleArn, }); - await this.ioHost.asIoHelper().defaults.info(chalk.green(`\n ✅ %s: ${action}ed`), chalk.blue(stack.displayName)); + await ioHelper.defaults.info(chalk.green(`\n ✅ %s: ${action}ed`), chalk.blue(stack.displayName)); } catch (e) { - await this.ioHost.asIoHelper().defaults.error(`\n ❌ %s: ${action} failed`, chalk.blue(stack.displayName), e); + await ioHelper.defaults.error(`\n ❌ %s: ${action} failed`, chalk.blue(stack.displayName), e); throw e; } } @@ -2103,22 +2102,10 @@ function buildParameterMap( */ async function askUserConfirmation( ioHost: CliIoHost, - concurrency: number, - motivation: string, - question: string, + req: ActionLessRequest, ) { await ioHost.withCorkedLogging(async () => { - // only talk to user if STDIN is a terminal (otherwise, fail) - if (!TESTING && !process.stdin.isTTY) { - throw new ToolkitError(`${motivation}, but terminal (TTY) is not attached so we are unable to get a confirmation from the user`); - } - - // only talk to user if concurrency is 1 (otherwise, fail) - if (concurrency > 1) { - throw new ToolkitError(`${motivation}, but concurrency is greater than 1 so we are unable to get a confirmation from the user`); - } - - const confirmed = await promptly.confirm(`${chalk.cyan(question)} (y/n)?`); + const confirmed = await ioHost.asIoHelper().requestResponse(req); if (!confirmed) { throw new ToolkitError('Aborted by user'); } diff --git a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts index 51d872a4d..45c611918 100644 --- a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts @@ -64,7 +64,6 @@ import type { DestroyStackResult } from '@aws-cdk/toolkit-lib/lib/api/deployment import { DescribeStacksCommand, GetTemplateCommand, StackStatus } from '@aws-sdk/client-cloudformation'; import { GetParameterCommand } from '@aws-sdk/client-ssm'; import * as fs from 'fs-extra'; -import * as promptly from 'promptly'; import type { Template, SdkProvider } from '../../lib/api'; import { Bootstrapper, type BootstrapSource } from '../../lib/api/bootstrap'; import type { @@ -81,7 +80,7 @@ import { import { Mode } from '../../lib/api/plugin'; import type { Tag } from '../../lib/api/tags'; import { asIoHelper } from '../../lib/api-private'; -import { CdkToolkit, markTesting } from '../../lib/cli/cdk-toolkit'; +import { CdkToolkit } from '../../lib/cli/cdk-toolkit'; import { CliIoHost } from '../../lib/cli/io-host'; import { Configuration } from '../../lib/cli/user-configuration'; import { StackActivityProgress } from '../../lib/commands/deploy'; @@ -99,14 +98,13 @@ import { } from '../_helpers/mock-sdk'; import { promiseWithResolvers } from '../_helpers/promises'; -markTesting(); - const defaultBootstrapSource: BootstrapSource = { source: 'default' }; const bootstrapEnvironmentMock = jest.spyOn(Bootstrapper.prototype, 'bootstrapEnvironment'); let cloudExecutable: MockCloudExecutable; let ioHost = CliIoHost.instance(); let ioHelper = asIoHelper(ioHost, 'deploy'); let notifySpy = jest.spyOn(ioHost, 'notify'); +let requestSpy = jest.spyOn(ioHost, 'requestResponse'); beforeEach(async () => { jest.resetAllMocks(); @@ -1700,7 +1698,8 @@ describe('rollback', () => { stackArn: 'stack:arn', }); - const mockedConfirm = jest.spyOn(promptly, 'confirm').mockResolvedValue(true); + // respond with yes + requestSpy.mockImplementationOnce(async () => true); const toolkit = new CdkToolkit({ ioHost, @@ -1725,9 +1724,9 @@ describe('rollback', () => { if (!useForce) { // Questions will have been asked only if --force is not specified if (firstResult.type === 'failpaused-need-rollback-first') { - expect(mockedConfirm).toHaveBeenCalledWith(expect.stringContaining('Roll back first and then proceed with deployment')); + expect(requestSpy).toHaveBeenCalledWith(expectIoMsg(expect.stringContaining('Roll back first and then proceed with deployment'))); } else { - expect(mockedConfirm).toHaveBeenCalledWith(expect.stringContaining('Perform a regular deployment')); + expect(requestSpy).toHaveBeenCalledWith(expectIoMsg(expect.stringContaining('Perform a regular deployment'))); } } From ad5f7e7f2011dccf1ffb404fbd37c5aaefc692ad Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Mon, 6 Oct 2025 11:31:58 +0200 Subject: [PATCH 2/2] fixup --- .../destroy/cdk-destroy-interactive.integtest.ts | 4 ++++ packages/aws-cdk/lib/cli/cdk-toolkit.ts | 15 ++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-interactive.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-interactive.integtest.ts index 3f319f113..8d2f10000 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-interactive.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-interactive.integtest.ts @@ -14,6 +14,10 @@ integTest('cdk destroy prompts the user for confirmation', withDefaultFixture(as interact: [ { prompt: /Are you sure you want to delete/, input: 'no' }, ], + modEnv: { + // disable coloring because it messes up prompt matching. + FORCE_COLOR: '0', + }, }); // assert we didn't destroy the stack diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index ce4544baa..322aadb57 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -977,9 +977,13 @@ export class CdkToolkit { if (!options.force) { const motivation = 'Destroying stacks is an irreversible action'; const question = `Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))}`; - const confirmed = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I7010.req(question, { motivation })); - if (!confirmed) { - await ioHelper.notify(IO.CDK_TOOLKIT_E7010.msg('Aborted by user')); + try { + await ioHelper.requestResponse(IO.CDK_TOOLKIT_I7010.req(question, { motivation })); + } catch (err: unknown) { + if (!ToolkitError.isToolkitError(err) || err.message != 'Aborted by user') { + throw err; // unexpected error + } + await ioHelper.notify(IO.CDK_TOOLKIT_E7010.msg(err.message)); return; } } @@ -2105,10 +2109,7 @@ async function askUserConfirmation( req: ActionLessRequest, ) { await ioHost.withCorkedLogging(async () => { - const confirmed = await ioHost.asIoHelper().requestResponse(req); - if (!confirmed) { - throw new ToolkitError('Aborted by user'); - } + await ioHost.asIoHelper().requestResponse(req); }); }