fix(cli): write notices to stderr or don't write them at all

rix0rrr · rix0rrr · commit 9fcb95aaf6c7 · 2025-03-04T11:28:42.000+01:00
On CI systems, the CDK CLI tries to avoid writing to `stderr` because there are a couple of CI systems that are commonly configured to fail if any output is written to `stderr`. That means all output, like notices, must go to `stdout`. Some commands (like `cdk synth` or `cdk bootstrap --show-template`) produce usable output on `stdout`, and these are commonly scripted, like piping their output to a file. However, because notices must go to `stdout`, these now interfere with the output of these commands. This needs a more thorough reworking of the CLI output streams, but there is a risk of affecting users who are currently relying on the fact that all output goes to `stdout`. In this PR, we are doing the first steps to solving this situation: - Notices will always go to `stderr`, so that they will never interfere with `stdout` anymore. - We try to detect what CI system we are running on, and we will completely suppress notices *unless* we determine that we are running on a CI system where it is "safe" to write to `sterr` (fail closed). "Safe" in this case means that the CI system doesn't come with an easy to toggle checkbox that makes commands fail based on what they print, instead of their exit codes. The only systems I'm aware of that have this checkbox are "Azure DevOps", and "TeamCity running PowerShell scripts". Even though we know the systems that are "unsafe", we will only show notices on systems known to be "safe". Fixes aws/aws-cdk#33589.
diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts
@@ -206,6 +206,28 @@ export const CODES = {
     level: 'error',
   }),
 
+  // Notices
+  CDK_TOOLKIT_I0100: codeInfo({
+    code: 'CDK_TOOLKIT_I0100',
+    description: 'Notices decoration (the header or footer of a list of notices)',
+    level: 'info',
+  }),
+  CDK_TOOLKIT_W0101: codeInfo({
+    code: 'CDK_TOOLKIT_W0101',
+    description: 'A notice that is marked as a warning',
+    level: 'warn',
+  }),
+  CDK_TOOLKIT_E0101: codeInfo({
+    code: 'CDK_TOOLKIT_E0101',
+    description: 'A notice that is marked as an error',
+    level: 'error',
+  }),
+  CDK_TOOLKIT_I0101: codeInfo({
+    code: 'CDK_TOOLKIT_I0101',
+    description: 'A notice that is marked as informational',
+    level: 'info',
+  }),
+
   // Assembly codes
   CDK_ASSEMBLY_I0042: codeInfo({
     code: 'CDK_ASSEMBLY_I0042',
diff --git a/packages/aws-cdk/lib/cli/ci-systems.ts b/packages/aws-cdk/lib/cli/ci-systems.ts
@@ -0,0 +1,83 @@
+
+interface CiSystem {
+  /**
+   * What's the name?
+   */
+  readonly name: string;
+
+  /**
+   * What environment variable indicates that we are running on this system?
+   */
+  readonly detectEnvVar: string;
+
+  /**
+   * Whether or not this CI system can be configured to fail on messages written to stderr
+   *
+   * With "can be configured", what we mean is that a checkbox or configuration
+   * flag to enable this behavior comes out of the box with the CI system and (judgement
+   * call), this flag is "commonly" used.
+   *
+   * Of course every CI system can be scripted to have this behavior, but that's
+   * not what we mean.
+   */
+  readonly canBeConfiguredToFailOnStdErr: boolean;
+}
+
+const CI_SYSTEMS: CiSystem[] = [
+  {
+    name: 'Azure DevOps',
+    // https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
+    detectEnvVar: 'TF_BUILD',
+    canBeConfiguredToFailOnStdErr: true,
+  },
+  {
+    name: 'TeamCity',
+    // https://www.jetbrains.com/help/teamcity/predefined-build-parameters.html
+    detectEnvVar: 'TEAMCITY_VERSION',
+    // Can be configured to fail on stderr, when using a PowerShell task
+    canBeConfiguredToFailOnStdErr: true,
+  },
+  {
+    name: 'GitHub Actions',
+    // https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
+    detectEnvVar: 'GITHUB_ACTION',
+    canBeConfiguredToFailOnStdErr: false,
+  },
+  {
+    name: 'CodeBuild',
+    // https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-env-vars.html
+    detectEnvVar: 'CODEBUILD_BUILD_ID',
+    canBeConfiguredToFailOnStdErr: false,
+  },
+  {
+    name: 'CircleCI',
+    // https://circleci.com/docs/variables/#built-in-environment-variables
+    detectEnvVar: 'CIRCLECI',
+    canBeConfiguredToFailOnStdErr: false,
+  },
+  {
+    name: 'Jenkins',
+    // https://www.jenkins.io/doc/book/pipeline/jenkinsfile/#using-environment-variables
+    detectEnvVar: 'EXECUTOR_NUMBER',
+    canBeConfiguredToFailOnStdErr: false,
+  },
+];
+
+export function detectCiSystem(): CiSystem | undefined {
+  for (const ciSystem of CI_SYSTEMS) {
+    if (process.env[ciSystem.detectEnvVar]) {
+      return ciSystem;
+    }
+  }
+  return undefined;
+}
+
+/**
+ * Return whether the CI system we're detecting is safe to write to stderr on
+ *
+ * Returns `undefined` if the current CI system cannot be recognized.
+ */
+export function ciSystemIsStdErrSafe(): boolean | undefined {
+  const x = detectCiSystem()?.canBeConfiguredToFailOnStdErr;
+  return x === undefined ? undefined : !x;
+}
diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts
@@ -2,6 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api';
 import '@jsii/check-node/run';
 import * as chalk from 'chalk';
 import { CdkToolkit, AssetBuildTime } from './cdk-toolkit';
+import { ciSystemIsStdErrSafe } from './ci-systems';
 import { parseCommandLineArguments } from './parse-command-line-arguments';
 import { checkForPlatformWarnings } from './platform-warnings';
 
@@ -90,10 +91,23 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
   });
   await configuration.load();
 
+  const shouldDisplayNotices = configuration.settings.get(['notices']);
+  if (shouldDisplayNotices !== undefined) {
+    // Notices either go to stderr, or nowhere
+    ioHost.noticesTarget = shouldDisplayNotices ? 'stderr' : 'drop';
+  } else {
+    // If the user didn't supply either `--notices` or `--no-notices`, we do
+    // autodetection. The autodetection currently is: do write notices if we are
+    // not on CI, or are on a CI system where we know that writing to stderr is
+    // safe. We fail "closed"; that is, we decide to NOT print for unknown CI
+    // systems, even though technically we maybe could.
+    const safeToWriteToStdErr = !argv.ci || Boolean(ciSystemIsStdErrSafe());
+    ioHost.noticesTarget = safeToWriteToStdErr ? 'stderr' : 'drop';
+  }
+
   const notices = Notices.create({
     context: configuration.context,
     output: configuration.settings.get(['outdir']),
-    shouldDisplay: configuration.settings.get(['notices']),
     includeAcknowledged: cmd === 'notices' ? !argv.unacknowledged : false,
     httpOptions: {
       proxyAddress: configuration.settings.get(['proxy']),
@@ -458,7 +472,12 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
 
       case 'notices':
         ioHost.currentAction = 'notices';
-        // This is a valid command, but we're postponing its execution
+        // If the user explicitly asks for notices, they are now the primary output
+        // of the command and they should go to stdout.
+        ioHost.noticesTarget = 'stdout';
+
+        // This is a valid command, but we're postponing its execution because displaying
+        // notices automatically happens after every command.
         return;
 
       case 'metadata':
diff --git a/packages/aws-cdk/lib/notices.ts b/packages/aws-cdk/lib/notices.ts
@@ -36,13 +36,6 @@ export interface NoticesProps {
    */
   readonly output?: string;
 
-  /**
-   * Global CLI option for whether we show notices
-   *
-   * @default true
-   */
-  readonly shouldDisplay?: boolean;
-
   /**
    * Options for the HTTP request
    */
@@ -298,7 +291,6 @@ export class Notices {
 
   private readonly context: Context;
   private readonly output: string;
-  private readonly shouldDisplay: boolean;
   private readonly acknowledgedIssueNumbers: Set<Number>;
   private readonly includeAcknowlegded: boolean;
   private readonly httpOptions: SdkHttpOptions;
@@ -313,7 +305,6 @@ export class Notices {
     this.acknowledgedIssueNumbers = new Set(this.context.get('acknowledged-issue-numbers') ?? []);
     this.includeAcknowlegded = props.includeAcknowledged ?? false;
     this.output = props.output ?? 'cdk.out';
-    this.shouldDisplay = props.shouldDisplay ?? true;
     this.httpOptions = props.httpOptions ?? {};
   }
 
@@ -339,10 +330,6 @@ export class Notices {
    * If context is configured to not display notices, this will no-op.
    */
   public async refresh(options: NoticesRefreshOptions = {}) {
-    if (!this.shouldDisplay) {
-      return;
-    }
-
     try {
       const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.httpOptions);
       const dataSource = new CachedDataSource(CACHE_FILE_PATH, underlyingDataSource, options.force ?? false);
@@ -357,10 +344,6 @@ export class Notices {
    * Display the relevant notices (unless context dictates we shouldn't).
    */
   public display(options: NoticesPrintOptions = {}) {
-    if (!this.shouldDisplay) {
-      return;
-    }
-
     const filteredNotices = NoticesFilter.filter({
       data: Array.from(this.data),
       cliVersion: versionNumber(),
@@ -369,29 +352,39 @@ export class Notices {
     });
 
     if (filteredNotices.length > 0) {
-      info('');
-      info('NOTICES         (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)');
-      info('');
+      info({
+        code: 'CDK_TOOLKIT_I0100',
+        message: [
+          '',
+          'NOTICES         (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)',
+          '',
+        ].join('\n'),
+      });
       for (const filtered of filteredNotices) {
-        const formatted = filtered.format();
+        const formatted = filtered.format() + '\n';
         switch (filtered.notice.severity) {
           case 'warning':
-            warning(formatted);
+            warning({ code: 'CDK_TOOLKIT_W0101', message: formatted });
             break;
           case 'error':
-            error(formatted);
+            error({ code: 'CDK_TOOLKIT_E0101', message: formatted });
             break;
           default:
-            info(formatted);
+            info({ code: 'CDK_TOOLKIT_I0101', message: formatted });
+            break;
         }
-        info('');
       }
-      info(`If you don’t want to see a notice anymore, use "cdk acknowledge <id>". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".`);
+      info({
+        code: 'CDK_TOOLKIT_I0100',
+        message: `If you don’t want to see a notice anymore, use "cdk acknowledge <id>". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".`
+      });
     }
 
     if (options.showTotal ?? false) {
-      info('');
-      info(`There are ${filteredNotices.length} unacknowledged notice(s).`);
+      info({
+        code: 'CDK_TOOLKIT_I0100',
+        message: `\nThere are ${filteredNotices.length} unacknowledged notice(s).`,
+      });
     }
   }
 }
diff --git a/packages/aws-cdk/lib/toolkit/cli-io-host.ts b/packages/aws-cdk/lib/toolkit/cli-io-host.ts
@@ -170,6 +170,11 @@ export interface CliIoHostProps {
   readonly stackProgress?: StackActivityProgress;
 }
 
+/**
+ * A type for configuring a target stream
+ */
+export type TargetStream = 'stdout' | 'stderr' | 'drop';
+
 /**
  * A simple IO host for the CLI that writes messages to the console.
  */
@@ -189,6 +194,14 @@ export class CliIoHost implements IIoHost {
    */
   private static _instance: CliIoHost | undefined;
 
+  /**
+   * Configure the target stream for notices
+   *
+   * (Not a setter because there's no need for additional logic when this value
+   * is changed yet)
+   */
+  public noticesTarget: TargetStream = 'stderr';
+
   // internal state for getters/setter
   private _currentAction: ToolkitAction;
   private _isCI: boolean;
@@ -379,8 +392,8 @@ export class CliIoHost implements IIoHost {
     }
 
     const output = this.formatMessage(msg);
-    const stream = this.selectStream(msg.level);
-    stream.write(output);
+    const stream = this.selectStream(msg);
+    stream?.write(output);
   }
 
   /**
@@ -394,10 +407,21 @@ export class CliIoHost implements IIoHost {
     ].includes(msg.code);
   }
 
+  /**
+   * Determines the output stream, based on message and configuration.
+   */
+  private selectStream(msg: IoMessage<any>): NodeJS.WriteStream | undefined {
+    if (isNoticesMessage(msg)) {
+      return targetStreamObject(this.noticesTarget);
+    }
+
+    return this.selectStreamFromLevel(msg.level);
+  }
+
   /**
    * Determines the output stream, based on message level and configuration.
    */
-  private selectStream(level: IoMessageLevel) {
+  private selectStreamFromLevel(level: IoMessageLevel): NodeJS.WriteStream {
     // The stream selection policy for the CLI is the following:
     //
     //   (1) Messages of level `result` always go to `stdout`
@@ -506,7 +530,7 @@ export class CliIoHost implements IIoHost {
    */
   private makeActivityPrinter() {
     const props: ActivityPrinterProps = {
-      stream: this.selectStream('info'),
+      stream: this.selectStreamFromLevel('info'),
     };
 
     switch (this.stackProgress) {
@@ -568,3 +592,22 @@ export function isCI(): boolean {
   return process.env.CI !== undefined && process.env.CI !== 'false' && process.env.CI !== '0';
 }
 
+function targetStreamObject(x: TargetStream): NodeJS.WriteStream | undefined {
+  switch (x) {
+    case 'stderr':
+      return process.stderr;
+    case 'stdout':
+      return process.stdout;
+    case 'drop':
+      return undefined;
+  }
+}
+
+function isNoticesMessage(msg: IoMessage<any>) {
+  return [
+    'CDK_TOOLKIT_I0100',
+    'CDK_TOOLKIT_W0101',
+    'CDK_TOOLKIT_E0101',
+    'CDK_TOOLKIT_I0101',
+  ].includes(msg.code);
+}
