From dc807740efa31f39d860ed70414febe7382a73bf Mon Sep 17 00:00:00 2001 From: vincent de smet Date: Fri, 7 Aug 2026 18:18:04 +0700 Subject: [PATCH 1/3] fix(cli): fail cleanly when the --outputs-file write fails A failed `--outputs-file` write after a successful deploy crashed the CLI with a raw stack trace, having first told the user the file had been written. `DeployConfig.onOutputsRetrieved` is typed `(outputs) => void`, and a `void` return type silently accepts an `async` function, so TypeScript never flagged that `handlers.ts` wires it to the `async` `saveOutputs` while `runDeploy` calls it without `await`. A write failure therefore became a floating rejected promise that no `catch` could observe: `runDeploy` resolved normally, execution carried on to print "The outputs have been written to ", and the process then died on the unhandled rejection. Awaiting the callback and guarding it separately from the rendering, because the two failures deserve different treatment: * A failed outputs-file write stays fatal - the user explicitly asked for that file, and exiting 0 would let a pipeline consume a stale or absent one. It is now wrapped as an `External` error, which `cdktn.ts`'s `.fail()` prints as a single clean line instead of a stack trace and a crash report. * A failure only *rendering* the outputs table remains non-fatal, since the deploy and the write have both already succeeded by then. Splitting the guards also means a failing write no longer skips the outputs display, and the "written to" line is now only reachable once the write has actually succeeded. This is not a success -> failure change. Verified against published CLIs with a deploy whose outputs-file write fails: cdktf-cli 0.21.0 exits 7, cdktn-cli 0.23.4 exits 7, cdktn-cli 0.24.0 exits 1. (The two older ones ship Ink; the 7 is Node's "Internal Exception Handler Run-Time Failure" from the Sentry uncaughtException handler faulting during Ink teardown.) Relative to 0.24.0 the exit code is unchanged - only the message improves. All three printed the false "written to" line before crashing. Reproduced on both macOS and Linux. `cdktn output --outputs-file` had the identical fire-and-forget defect and gets the same treatment. `watch` never uses this callback. --- packages/cdktn-cli/src/bin/cmds/handlers.ts | 8 ++- .../src/bin/cmds/ui/__tests__/deploy.test.ts | 63 +++++++++++++++++++ packages/cdktn-cli/src/bin/cmds/ui/deploy.ts | 51 +++++++++++---- packages/cdktn-cli/src/bin/cmds/ui/output.ts | 50 +++++++++++---- 4 files changed, 145 insertions(+), 27 deletions(-) diff --git a/packages/cdktn-cli/src/bin/cmds/handlers.ts b/packages/cdktn-cli/src/bin/cmds/handlers.ts index 2bd396eb4..dcc66b43e 100644 --- a/packages/cdktn-cli/src/bin/cmds/handlers.ts +++ b/packages/cdktn-cli/src/bin/cmds/handlers.ts @@ -200,7 +200,9 @@ export async function deploy(argv: any) { let outputsPath: string | undefined = undefined; - let onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void = () => {}; + let onOutputsRetrieved: ( + outputs: NestedTerraformOutputs, + ) => void | Promise = () => {}; if (argv.outputsFile) { outputsPath = normalizeOutputPath(argv.outputsFile); @@ -523,7 +525,9 @@ export async function output(argv: any) { const skipProviderLock = argv.skipProviderLock; let outputsPath: string | undefined = undefined; - let onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void = () => {}; + let onOutputsRetrieved: ( + outputs: NestedTerraformOutputs, + ) => void | Promise = () => {}; if (argv.outputsFile) { outputsPath = normalizeOutputPath(argv.outputsFile); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts index e2627ab05..ce4702a45 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts @@ -264,6 +264,69 @@ describe("runDeploy output rendering is non-fatal", () => { }); }); +describe("runDeploy --outputs-file write failures are fatal", () => { + const outputsByConstructId = { + db: { host: { sensitive: false, type: "string", value: "db.example.com" } }, + }; + + beforeEach(() => { + mockRunCdktfProject.mockImplementation(async () => ({ + returnValue: undefined, + project: { outputsByConstructId }, + })); + }); + + it("rejects with a clean External error when the write rejects asynchronously", async () => { + // Mirrors handlers.ts wiring: `onOutputsRetrieved` is `saveOutputs`, an async function. If this + // call is not awaited, a rejection here becomes a floating, unhandled promise rejection instead + // of something `runDeploy` itself rejects with - this assertion (runDeploy REJECTS) would fail + // against code that doesn't await the call, since that code resolves normally instead. + const onOutputsRetrieved = jest + .fn() + .mockRejectedValue(new Error("ENOENT: no such file or directory")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + // Clean/fatal means typed "External" (see cdktn.ts's `.fail()` handler: External/Usage errors + // print just `error.message`, everything else prints message + stack + "Collecting Debug + // Information..."), not merely "something was thrown". + expect(caught.__type).toBe("External"); + expect(caught.message).toContain("ENOENT: no such file or directory"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); + + it("does not print the 'written to' line when the write fails", async () => { + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).not.toContain("The outputs have been written to"); + + logSpy.mockRestore(); + }); +}); + describe("runDeploy sentinel override routing", () => { it("routes an 'override' answer to status.override()", async () => { const override = jest.fn(); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts b/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts index 253eaf6bd..942862f99 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts @@ -1,5 +1,6 @@ // Copyright (c) HashiCorp, Inc // SPDX-License-Identifier: MPL-2.0 +import { Errors } from "@cdktn/commons"; import { NestedTerraformOutputs } from "@cdktn/cli-core"; import { runCdktfProject, Status } from "../helper/project-runner"; import { StreamRenderer } from "../helper/tty-stream"; @@ -16,7 +17,7 @@ export interface DeployConfig { targetStacks?: string[]; synthCommand: string; autoApprove: boolean; - onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void; + onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void | Promise; outputsPath?: string; ignoreMissingStackDependencies?: boolean; parallelism?: number; @@ -38,10 +39,14 @@ export interface DeployConfig { * `status.stop()` so cli-core halts cleanly rather than hanging. * * @param config - All deploy options, forwarded near-verbatim to `CdktfProject.deploy`. `onOutputsRetrieved` is - * invoked once the run completes (or is stopped) with the final outputs map. + * invoked once the run completes (or is stopped) with the final outputs map, after the outputs + * table has already been rendered; it may return a promise (e.g. writing --outputs-file to + * disk), which is awaited and, if it rejects, surfaced as a fatal error since a broken outputs + * write is a broken promise to the caller. * @returns Promise that resolves when the deploy completes, is stopped, or is dismissed. Rejects with whatever - * cli-core rejects with for real failures (terraform error, abort signal, etc.). A failure *rendering* - * the fetched outputs is non-fatal and does not cause a rejection. + * cli-core rejects with for real failures (terraform error, abort signal, etc.), or with a Usage error + * (bad --outputs-file path, e.g. ENOENT/ENOTDIR) or External error (any other `onOutputsRetrieved` + * failure). A failure only *rendering* the fetched outputs is non-fatal and does not cause a rejection. */ export async function runDeploy({ outDir, @@ -160,8 +165,9 @@ export async function runDeploy({ const outputs = project.outputsByConstructId; - onOutputsRetrieved(outputs); - + // Render the outputs table first (still non-fatal): the deploy already succeeded, so a + // failure here is purely cosmetic, and rendering before the --outputs-file write below means + // the user still sees their outputs even if that write fails. let rendered = ""; let renderFailed = false; try { @@ -180,19 +186,38 @@ export async function runDeploy({ if (rendered) { console.log(rendered); - } - - if (rendered || renderFailed) { - if (outputsPath) { - console.log(`The outputs have been written to ${outputsPath}`); - } - } else { + } else if (!renderFailed) { // Either there were no declared outputs at all, or every one of them was dropped upstream // (e.g. all missing from `terraform output`, the empty-group case renderOutputs collapses // to ""). Either way there is nothing to show the user, so say so plainly instead of // printing a stray blank line. console.log("No outputs found."); } + + // A failed --outputs-file write is a broken promise to the user and must be fatal: await it + // and rethrow as a clean error, which cdktn.ts's top-level `.fail()` handler prints as a + // single clean line rather than a stack trace, since the deploy itself already succeeded (and + // its outputs were already rendered above, so the write failure below does not hide them). + // ENOENT/ENOTDIR means the user pointed --outputs-file at a path that doesn't exist - a usage + // mistake, not something outside our control - so it is reported as a Usage error (excluded + // from Sentry crash reporting) rather than External. + try { + await onOutputsRetrieved(outputs); + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code; + const ErrorCtor = + code === "ENOENT" || code === "ENOTDIR" + ? Errors.Usage + : Errors.External; + throw ErrorCtor( + `Failed to write outputs: ${e instanceof Error ? e.message : e}`, + e instanceof Error ? e : undefined, + ); + } + + if (outputsPath) { + console.log(`The outputs have been written to ${outputsPath}`); + } } finally { stream.stop(); } diff --git a/packages/cdktn-cli/src/bin/cmds/ui/output.ts b/packages/cdktn-cli/src/bin/cmds/ui/output.ts index 57ae5a866..06d86e461 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/output.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/output.ts @@ -1,5 +1,6 @@ // Copyright (c) HashiCorp, Inc // SPDX-License-Identifier: MPL-2.0 +import { Errors } from "@cdktn/commons"; import { NestedTerraformOutputs } from "@cdktn/cli-core"; import { runCdktfProject, Status } from "../helper/project-runner"; import { StreamRenderer } from "../helper/tty-stream"; @@ -10,7 +11,7 @@ export interface OutputConfig { outDir: string; targetStacks?: string[]; synthCommand: string; - onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void; + onOutputsRetrieved: (outputs: NestedTerraformOutputs) => void | Promise; outputsPath?: string; skipSynth?: boolean; skipProviderLock?: boolean; @@ -39,8 +40,11 @@ function statusBar(status: Status): string { * Drive a `cdktn output` invocation. Fetches Terraform outputs (optionally skipping synth/provider-lock), prints them * in nested form, and writes them to disk when `outputsPath` is provided. * - * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs before they are printed. - * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. + * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs before they are printed; + * it may return a promise (e.g. writing --outputs-file to disk), which is awaited and, if it + * rejects, surfaced as a fatal External error (see the matching guard in `runDeploy`). + * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. A failure only + * *rendering* the fetched outputs is non-fatal and does not cause a rejection. */ export async function runOutput({ outDir, @@ -55,7 +59,7 @@ export async function runOutput({ stream.start(); try { - const { returnValue } = await runCdktfProject( + const { returnValue: outputs } = await runCdktfProject( { outDir, synthCommand, @@ -68,20 +72,42 @@ export async function runOutput({ }); }, }, - async (project) => { - const outputs = await project.fetchOutputs({ + (project) => + project.fetchOutputs({ stackNames: targetStacks, skipSynth, skipProviderLock, - }); - onOutputsRetrieved(outputs); - return outputs; - }, + }), ); stream.clearBar(); - if (returnValue && Object.keys(returnValue).length > 0) { - console.log(renderOutputs(returnValue)); + + // See runDeploy() in ./deploy.ts for the rationale: a failed --outputs-file write must be + // fatal, wrapped as an External error so cdktn.ts's top-level `.fail()` handler prints a + // single clean line instead of a stack trace. + try { + await onOutputsRetrieved(outputs); + } catch (e) { + throw Errors.External( + `Failed to write outputs${outputsPath ? ` to ${outputsPath}` : ""}: ${ + e instanceof Error ? e.message : e + }`, + e instanceof Error ? e : undefined, + ); + } + + if (outputs && Object.keys(outputs).length > 0) { + try { + console.log(renderOutputs(outputs)); + } catch (e) { + // The fetch - and the outputs write above - already succeeded at this point; a failure + // rendering the outputs table is purely cosmetic and must not fail the command. + console.error( + `\nOutputs fetched, but rendering them failed: ${ + e instanceof Error ? e.message : e + }`, + ); + } if (outputsPath) { console.log(`The outputs have been written to ${outputsPath}`); } From 2b03546f67ecf5f6172860838065229460b16ac5 Mon Sep 17 00:00:00 2001 From: vincent de smet Date: Fri, 7 Aug 2026 21:50:46 +0700 Subject: [PATCH 2/3] fix(cli): print outputs before failing on a bad --outputs-file path runOutput wrote --outputs-file before rendering the outputs table, so a failing write (mistyped path) skipped the table the same way runDeploy used to before its own reordering during the rebase resolution. Apply the same fix here: render first (still non-fatal), then await the save and throw on failure, printing "written to " only once the save actually succeeds. Classify the save failure by errno instead of by message text. ENOENT/ENOTDIR means the user pointed --outputs-file at a path that doesn't exist - a usage mistake, not something outside our control - so it is now reported as Errors.Usage (excluded from Sentry crash reporting) in both runDeploy and runOutput; other fs errors (EACCES, ENOSPC, ...) stay External. Also drop the path from the "Failed to write outputs to X: ENOENT: ... open 'X'" message - the fs error already names the path, so the prefix no longer repeats it. Adds direct coverage for runOutput's error-wrapping path (previously untested): Usage vs External classification, the outputs table printing before a save failure, and the "written to" line staying unreachable on failure. --- .../src/bin/cmds/ui/__tests__/deploy.test.ts | 69 +++++- .../src/bin/cmds/ui/__tests__/output.test.ts | 206 ++++++++++++++++++ packages/cdktn-cli/src/bin/cmds/ui/output.ts | 72 +++--- 3 files changed, 311 insertions(+), 36 deletions(-) create mode 100644 packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts index ce4702a45..ade7ad2cd 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/deploy.test.ts @@ -276,14 +276,16 @@ describe("runDeploy --outputs-file write failures are fatal", () => { })); }); - it("rejects with a clean External error when the write rejects asynchronously", async () => { + it("rejects with a clean Usage error when the write fails with ENOENT (bad path)", async () => { // Mirrors handlers.ts wiring: `onOutputsRetrieved` is `saveOutputs`, an async function. If this // call is not awaited, a rejection here becomes a floating, unhandled promise rejection instead // of something `runDeploy` itself rejects with - this assertion (runDeploy REJECTS) would fail // against code that doesn't await the call, since that code resolves normally instead. - const onOutputsRetrieved = jest - .fn() - .mockRejectedValue(new Error("ENOENT: no such file or directory")); + const err: NodeJS.ErrnoException = new Error( + "ENOENT: no such file or directory, open '/missing-dir/out.json'", + ); + err.code = "ENOENT"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); let caught: any; @@ -298,16 +300,45 @@ describe("runDeploy --outputs-file write failures are fatal", () => { } expect(caught).toBeDefined(); - // Clean/fatal means typed "External" (see cdktn.ts's `.fail()` handler: External/Usage errors - // print just `error.message`, everything else prints message + stack + "Collecting Debug - // Information..."), not merely "something was thrown". - expect(caught.__type).toBe("External"); + // A bad --outputs-file path is a usage mistake, not something outside our control: it must be + // typed "Usage", not "External" (see cdktn.ts's `.fail()` handler: External/Usage errors print + // just `error.message`, everything else prints message + stack + "Collecting Debug + // Information..."; Usage errors are also excluded from Sentry crash reporting). + expect(caught.__type).toBe("Usage"); expect(caught.message).toContain("ENOENT: no such file or directory"); + // The fs error already names the path; the prefix must not repeat it. + expect(caught.message).not.toContain("to /missing-dir/out.json:"); expect(mockStreamStop).toHaveBeenCalledTimes(1); logSpy.mockRestore(); }); + it("rejects with a clean External error when the write fails with EACCES (permission denied)", async () => { + const err: NodeJS.ErrnoException = new Error( + "EACCES: permission denied, open '/missing-dir/out.json'", + ); + err.code = "EACCES"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("External"); + expect(caught.message).toContain("EACCES: permission denied"); + + logSpy.mockRestore(); + }); + it("does not print the 'written to' line when the write fails", async () => { const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); @@ -325,6 +356,28 @@ describe("runDeploy --outputs-file write failures are fatal", () => { logSpy.mockRestore(); }); + + it("still prints the outputs table before rejecting on a write failure", async () => { + // The deploy succeeded and the outputs already exist in memory; only persistence failed. The + // table must reach the user before the rejection, not be swallowed by the failing write. + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runDeploy({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = stripAnsi( + logSpy.mock.calls.map((call) => call[0]).join("\n"), + ); + expect(printed).toContain("host = db.example.com"); + + logSpy.mockRestore(); + }); }); describe("runDeploy sentinel override routing", () => { diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts new file mode 100644 index 000000000..a28f836f8 --- /dev/null +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) HashiCorp, Inc +// SPDX-License-Identifier: MPL-2.0 +import stripAnsi from "strip-ansi"; + +// Mock the project runner so we can drive a synthetic result without spawning a real CdktfProject. +// Unlike deploy.ts (which reads `project.outputsByConstructId` after the run), output.ts's +// `runOutput` gets its outputs from `runCdktfProject`'s `returnValue`, which is whatever the +// callback passed to `runCdktfProject` resolves to - here, `project.fetchOutputs(...)`. +const mockRunCdktfProject = jest.fn(); +jest.mock("../../helper/project-runner", () => ({ + runCdktfProject: (opts: unknown, cb: unknown) => + mockRunCdktfProject(opts, cb), +})); + +// Suppress noise from the StreamRenderer in unit tests, but keep a handle on `stop` so tests can +// assert it always runs regardless of which path (fatal save error / non-fatal render error) is hit. +const mockStreamStop = jest.fn(); +jest.mock("../../helper/tty-stream", () => ({ + StreamRenderer: jest.fn().mockImplementation(() => ({ + start: jest.fn(), + stop: mockStreamStop, + setBar: jest.fn(), + clearBar: jest.fn(), + appendLog: jest.fn(), + })), +})); + +// renderOutputs defaults to the real implementation; individual tests override it via +// mockRenderOutputs.mockImplementationOnce(...) to simulate a rendering failure. +const actualFormat = jest.requireActual("../../helper/format"); +const mockRenderOutputs = jest.fn(actualFormat.renderOutputs); +jest.mock("../../helper/format", () => { + const actual = jest.requireActual("../../helper/format"); + return { + ...actual, + renderOutputs: (...args: unknown[]) => mockRenderOutputs(...args), + }; +}); + +import { runOutput } from "../output"; + +const baseConfig = { + outDir: "out", + synthCommand: "noop", + onOutputsRetrieved: () => {}, +}; + +const outputsByConstructId = { + db: { host: { sensitive: false, type: "string", value: "db.example.com" } }, +}; + +beforeEach(() => { + mockRunCdktfProject.mockReset(); + mockStreamStop.mockReset(); + mockRenderOutputs.mockReset(); + mockRenderOutputs.mockImplementation(actualFormat.renderOutputs); + mockRunCdktfProject.mockImplementation(async () => ({ + returnValue: outputsByConstructId, + project: {}, + })); +}); + +describe("runOutput --outputs-file write failures are fatal", () => { + it("rejects with a clean Usage error when the write fails with ENOENT (bad path)", async () => { + // Mirrors handlers.ts wiring: `onOutputsRetrieved` is `saveOutputs`, an async function. If this + // call is not awaited, a rejection here becomes a floating, unhandled promise rejection instead + // of something `runOutput` itself rejects with - this assertion (runOutput REJECTS) would fail + // against code that doesn't await the call, since that code resolves normally instead. + const err: NodeJS.ErrnoException = new Error( + "ENOENT: no such file or directory, open '/missing-dir/out.json'", + ); + err.code = "ENOENT"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("Usage"); + expect(caught.message).toContain("ENOENT: no such file or directory"); + // The fs error already names the path; the prefix must not repeat it. + expect(caught.message).not.toContain("to /missing-dir/out.json:"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); + + it("rejects with a clean External error when the write fails with EACCES (permission denied)", async () => { + const err: NodeJS.ErrnoException = new Error( + "EACCES: permission denied, open '/missing-dir/out.json'", + ); + err.code = "EACCES"; + const onOutputsRetrieved = jest.fn().mockRejectedValue(err); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + let caught: any; + try { + await runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any); + } catch (e) { + caught = e; + } + + expect(caught).toBeDefined(); + expect(caught.__type).toBe("External"); + expect(caught.message).toContain("EACCES: permission denied"); + + logSpy.mockRestore(); + }); + + it("does not print the 'written to' line when the write fails", async () => { + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).not.toContain("The outputs have been written to"); + + logSpy.mockRestore(); + }); + + it("still prints the outputs table before rejecting on a write failure", async () => { + // The fetch succeeded and the outputs already exist in memory; only persistence failed. The + // table must reach the user before the rejection, not be swallowed by the failing write. + const onOutputsRetrieved = jest.fn().mockRejectedValue(new Error("boom")); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/missing-dir/out.json", + } as any), + ).rejects.toBeDefined(); + + const printed = stripAnsi( + logSpy.mock.calls.map((call) => call[0]).join("\n"), + ); + expect(printed).toContain("host = db.example.com"); + + logSpy.mockRestore(); + }); + + it("resolves and prints the 'written to' line when the write succeeds", async () => { + const onOutputsRetrieved = jest.fn().mockResolvedValue(undefined); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + + await expect( + runOutput({ + ...baseConfig, + onOutputsRetrieved, + outputsPath: "/tmp/out.json", + } as any), + ).resolves.toBeUndefined(); + + expect(onOutputsRetrieved).toHaveBeenCalledWith(outputsByConstructId); + const printed = logSpy.mock.calls.map((call) => call[0]).join("\n"); + expect(printed).toContain("The outputs have been written to /tmp/out.json"); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + }); +}); + +describe("runOutput output rendering is non-fatal", () => { + it("does not fail the command when rendering the outputs throws", async () => { + mockRenderOutputs.mockImplementationOnce(() => { + throw new Error("render boom"); + }); + const onOutputsRetrieved = jest.fn(); + const logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + const errSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + await expect( + runOutput({ ...baseConfig, onOutputsRetrieved } as any), + ).resolves.toBeUndefined(); + + expect(onOutputsRetrieved).toHaveBeenCalledWith(outputsByConstructId); + expect(errSpy).toHaveBeenCalledWith( + expect.stringContaining("Outputs fetched, but rendering them failed"), + ); + expect(mockStreamStop).toHaveBeenCalledTimes(1); + + logSpy.mockRestore(); + errSpy.mockRestore(); + }); +}); diff --git a/packages/cdktn-cli/src/bin/cmds/ui/output.ts b/packages/cdktn-cli/src/bin/cmds/ui/output.ts index 06d86e461..a60399771 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/output.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/output.ts @@ -40,11 +40,13 @@ function statusBar(status: Status): string { * Drive a `cdktn output` invocation. Fetches Terraform outputs (optionally skipping synth/provider-lock), prints them * in nested form, and writes them to disk when `outputsPath` is provided. * - * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs before they are printed; - * it may return a promise (e.g. writing --outputs-file to disk), which is awaited and, if it - * rejects, surfaced as a fatal External error (see the matching guard in `runDeploy`). - * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. A failure only - * *rendering* the fetched outputs is non-fatal and does not cause a rejection. + * @param config - Output options. `onOutputsRetrieved` is called with the fetched outputs, after they have already + * been printed; it may return a promise (e.g. writing --outputs-file to disk), which is awaited + * and, if it rejects, surfaced as a fatal error (see the matching guard in `runDeploy`). + * @returns Promise that resolves when outputs have been fetched and printed, rejects on failure. Rejects with a + * Usage error (bad --outputs-file path, e.g. ENOENT/ENOTDIR) or External error (any other + * `onOutputsRetrieved` failure). A failure only *rendering* the fetched outputs is non-fatal and does + * not cause a rejection. */ export async function runOutput({ outDir, @@ -82,38 +84,52 @@ export async function runOutput({ stream.clearBar(); - // See runDeploy() in ./deploy.ts for the rationale: a failed --outputs-file write must be - // fatal, wrapped as an External error so cdktn.ts's top-level `.fail()` handler prints a - // single clean line instead of a stack trace. + // Render the outputs table first (still non-fatal): the fetch already succeeded, so a + // failure here is purely cosmetic, and rendering before the --outputs-file write below means + // the user still sees their outputs even if that write fails. + let rendered = ""; + let renderFailed = false; try { - await onOutputsRetrieved(outputs); + rendered = outputs ? renderOutputs(outputs) : ""; } catch (e) { - throw Errors.External( - `Failed to write outputs${outputsPath ? ` to ${outputsPath}` : ""}: ${ + // The fetch already succeeded at this point; a failure rendering the outputs table is + // purely cosmetic and must not fail the command. Log it so the user still learns something + // went wrong, but do not rethrow. + console.error( + `\nOutputs fetched, but rendering them failed: ${ e instanceof Error ? e.message : e }`, - e instanceof Error ? e : undefined, ); + renderFailed = true; } - if (outputs && Object.keys(outputs).length > 0) { - try { - console.log(renderOutputs(outputs)); - } catch (e) { - // The fetch - and the outputs write above - already succeeded at this point; a failure - // rendering the outputs table is purely cosmetic and must not fail the command. - console.error( - `\nOutputs fetched, but rendering them failed: ${ - e instanceof Error ? e.message : e - }`, - ); - } - if (outputsPath) { - console.log(`The outputs have been written to ${outputsPath}`); - } - } else { + if (rendered) { + console.log(rendered); + } else if (!renderFailed) { console.log("No outputs found."); } + + // See runDeploy() in ./deploy.ts for the rationale: a failed --outputs-file write must be + // fatal, wrapped as a clean error so cdktn.ts's top-level `.fail()` handler prints a single + // clean line instead of a stack trace, since the outputs were already rendered above. A bad + // path (ENOENT/ENOTDIR) is a usage mistake and reported as Usage, not External. + try { + await onOutputsRetrieved(outputs); + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code; + const ErrorCtor = + code === "ENOENT" || code === "ENOTDIR" + ? Errors.Usage + : Errors.External; + throw ErrorCtor( + `Failed to write outputs: ${e instanceof Error ? e.message : e}`, + e instanceof Error ? e : undefined, + ); + } + + if (outputsPath) { + console.log(`The outputs have been written to ${outputsPath}`); + } } finally { stream.stop(); } From c35fcba41da6593831248f0683786fd6e78874b7 Mon Sep 17 00:00:00 2001 From: vincent de smet Date: Fri, 7 Aug 2026 22:09:58 +0700 Subject: [PATCH 3/3] test(cli): exercise fetchOutputs in the runOutput tests The mock resolved a returnValue without ever invoking the project callback, so `project.fetchOutputs` never ran and the suite would still have passed if runOutput stopped fetching outputs entirely. --- .../cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts index a28f836f8..14ae9eb16 100644 --- a/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts +++ b/packages/cdktn-cli/src/bin/cmds/ui/__tests__/output.test.ts @@ -54,8 +54,13 @@ beforeEach(() => { mockStreamStop.mockReset(); mockRenderOutputs.mockReset(); mockRenderOutputs.mockImplementation(actualFormat.renderOutputs); - mockRunCdktfProject.mockImplementation(async () => ({ - returnValue: outputsByConstructId, + // Actually invoke the project callback so `project.fetchOutputs` is exercised: runOutput's + // returnValue is whatever that callback resolves to, and a mock that skips it would still pass + // if runOutput stopped fetching outputs altogether. + mockRunCdktfProject.mockImplementation(async (_opts, projectCallback) => ({ + returnValue: await projectCallback({ + fetchOutputs: jest.fn().mockResolvedValue(outputsByConstructId), + }), project: {}, })); });