[so0k]

Related issue

No GitHub issue. Found while reviewing #375; it is a separate crash on a different code path.

Stacked on #375. This PR targets fix/deploy-outputs-undefined-crash, so the diff shown here is only this change. Review #375 first.

Description

A failed --outputs-file write after an otherwise successful deploy crashed the CLI with a raw stack trace, after telling the user the file had been written.

Root cause

onOutputsRetrieved was typed (outputs) => void. A void return type accepts an async function, so TypeScript never flagged that handlers.ts wires it to the async saveOutputs while the caller did not await it:

onOutputsRetrieved(outputs); // returns a promise nobody holds

A write failure became a rejected promise nobody observed. The command carried on, printed The outputs have been written to <path>, and the process then died on the unhandled rejection. cdktn output --outputs-file had the same defect; watch never uses this callback.

This is not a success-to-failure change

A failing --outputs-file write has always been fatal, through a crash. Measured against published CLIs:

CLI	Exit code	Stack trace	False "written to" line
cdktf-cli@0.21.0	7	yes	yes
cdktn-cli@0.23.4	7	yes	yes
cdktn-cli@0.24.0	1	yes	yes

No version exits 0, so no flag or opt-in is introduced. Relative to 0.24.0 the exit code is unchanged; the message improves.

The fix

All of it lives in presentOutputs (cmds/helper/outputs.ts, introduced in #375), which both runDeploy and runOutput now await. The order is:

1. print the outputs table (or No outputs found.);
2. await the write;
3. print the written to line.

So the values stay on screen when the write fails, and the written to line can never follow a failed write.

A rejected write is rethrown as a typed error, Failed to write outputs: <original message>:

• ENOENT / ENOTDIR (the path does not exist), EISDIR (it names an existing directory) and ENAMETOOLONG are Usage errors: printed as one line, not captured by Sentry (bin/error-handling.ts);
• anything else (EACCES, ENOSPC, …) is an External error: printed as one line and captured.

The callback type is widened to void | Promise<void> in handlers.ts and the two command configs.

Tests

• Helper: the write is awaited before the written to line; ENOENT, ENOTDIR, EISDIR and ENAMETOOLONG give Usage, EACCES gives External; the table is printed before the rejection; no written to line after a failure. Removing the await fails by assertion.
• One test goes through the real saveOutputs into a temp directory whose parent does not exist.
• Per command: one thin test that the rejection propagates and the stream is stopped. The rejected write is handled up front in every layer, so a dropped await fails an assertion instead of surfacing as an unhandled rejection.

Checklist

• I have updated the PR title to match CDKTN's style guide
• I have run the linter on my code locally
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation if applicable — n/a, the exit-code contract is unchanged
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works if applicable
• New and existing unit tests pass locally with my changes

[jsteinich]

Root cause confirmed, and deploy/output are indeed the only two commands using the callback.

The description and the code disagree on one point.

Splitting the guards also means a failing write no longer skips the outputs display

The throw Errors.External(...) sits before the render block, so a failing write does skip the display:

cdk-terrain/packages/cdktn-cli/src/bin/cmds/ui/deploy.ts

Lines 167 to 183 in d301821

	// A failed --outputs-file write is a broken promise to the user and must be fatal: await it and
	// rethrow as an External 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.
	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));

(same ordering in runOutput at L88-L101). Against 0.24.0's actual async behaviour, the table is now skipped where it previously printed before the crash.

Which did you mean?

• Leftover from an earlier draft → drop the sentence.
• You want the table to still print → the throw moves below the render block (collect, render, throw), plus a test.

Failing fast is defensible; I'd just like the two to agree.

Two smaller things:

1. ENOENT on a user-named directory reads more like Errors.Usage than External. Same clean-print branch either way, so semantics only — but EACCES/ENOSPC do fit External. Your call.
2. The path renders twice: Failed to write outputs to X: ENOENT: no such file or directory, open 'X'.

[so0k]

You're right that they disagree, and the answer is the second option — I want the table to print, so the
throw moves below the render block.

Not a leftover sentence: I'd written the description from the intent and then not checked it against the final
ordering, which is exactly the kind of thing a description should not get away with. Thanks for catching it.

The reasoning for render-then-throw: at that point the deploy has genuinely succeeded and the outputs exist in
memory. The only thing that failed is persisting them. Skipping the table means the user loses the values
entirely — they can't even copy them out of the terminal — for a failure that is often a mistyped --outputs-file
path they can just retry. Printing the table and then failing gives them both the data and the error. Your point
that this is a regression against 0.24.0's actual behaviour (where the table printed before the crash) makes it
worse than a wash, so failing fast here is losing information users had before.

New ordering: render → save → confirm. The written to line stays after the save, so it is still unreachable on
failure. With a test asserting the table is printed and the run rejects.

Errors.Usage for ENOENT — agreed, and it turns out to matter beyond semantics.

Usage errors are excluded from Sentry crash reporting, so classifying a user's mistyped path as External
would file a crash report for something that isn't a bug. Going with: ENOENT/ENOTDIR → Usage, everything
else (EACCES, ENOSPC, …) → External, keyed off err.code rather than message text.

Double path — fixing. The fs error message already names the path, so the prefix drops it:
Failed to write outputs: ENOENT: no such file or directory, open 'X'.

All three incoming; I'll push and re-request. The PR description gets corrected along with the code.

[so0k]

Pushed — rebased onto the updated #375, then 2b03546f6 + c35fcba41. All four points addressed.

• Render before failing. New ordering is render → save → confirm, in both runDeploy and runOutput. The
  written to line stays after the save so it's still unreachable on failure. Test asserts the table is
  printed and the run still rejects. The description sentence is now true rather than deleted.
• ENOENT/ENOTDIR → Errors.Usage, everything else stays External, keyed off err.code rather than
  message text. Tests cover both (ENOENT → Usage, EACCES → External).
• Path printed once — the prefix no longer repeats what the fs error already names.
• runOutput now has direct coverage (ui/__tests__/output.test.ts), modelled on its real shape —
  returnValue from the project callback, not outputsByConstructId.

Two notes against myself:

The first version of those runOutput tests passed for a partly wrong reason: the mock resolved a returnValue
without ever invoking the project callback, so fetchOutputs never ran and the suite would still have passed if
runOutput stopped fetching outputs altogether. Fixed in c35fcba41.

And a behaviour interaction worth knowing when reading the two PRs together: with --outputs-file and all
outputs dropped, #375 alone prints only No outputs found., whereas after this PR it prints that and the
written to line, since that line is now keyed off write success rather than render state.

[jsteinich]

Full pass over the rebased diff. This is much tighter than the previous revision now that the helper lives in #375 — what's left is exactly the defect plus the classification, and the render → write → confirm ordering means the written to line is genuinely unreachable on failure. The helper test that captures what had printed at the moment the write resolved is a nice way to pin the ordering rather than just asserting the end state.

One thing I'd ask for, inline, plus a test-only note.

Two verifications you may find useful:

The clean-exit argument is now stronger than the description says. It cites beforeSend dropping anything containing "Usage Error". Post-#410 that message-sniffing is no longer the mechanism: bin/error-handling.ts has an explicit three-way branch — Usage prints the message and is not captured (:53), External prints and is captured (:58), anything else prints message + stack + debug info. So the Usage/External split is now load-bearing structurally, not incidentally.

The telemetry paragraph in the description is stale and should come out. It says Errors.External fires an unawaited POST to checkpoint-api.hashicorp.com and flags it as a live data point for #62. After #411, errors.ts no longer imports ./checkpoint at all — it calls sendErrorTelemetry from ./telemetry. That POST is gone from the error path, so the note now points at something that doesn't happen. (#411 also fixed the latent bug where each Errors.* factory captured errorScope at module load; the scope is read at call time now.)

[so0k]

On the review-level notes: the checkpoint/telemetry paragraph is already gone from the body, and the Usage/External description now points at the explicit branch in bin/error-handling.ts (Usage printed and not captured, External printed and captured) rather than the old beforeSend message sniffing. The inline points are addressed in e32aa3e, 72a3798 and 10dc892, and the stack is rebased onto main (ea82124).

[jsteinich]

EISDIR/ENAMETOOLONG classification and the test-layer consistency look good. Approving to land together with #375.