fix(cli): don't crash after a successful deploy when an output is missing

[so0k]

Related issue

No GitHub issue — this was reported by a user through Sentry (CDKTN-5) on cdktn-cli 0.24.0, not filed as an issue. Happy to open one retroactively if you'd prefer the paper trail.

Related: #361 — the top-level .fail() handler is the common exit path that turned this throw into an unhandled rejection with a stack trace. Deliberately not fixed here; it deserves its own PR and a regression test.

Description

A user's cdktn deploy applied both stacks successfully and then crashed:

TypeError: Cannot convert undefined or null to object
    at Object.entries (<anonymous>)
    at renderNested (helper/format.ts:126)   <- twice, nested
    at renderOutputs (helper/format.ts:142)
    at runDeploy (ui/deploy.ts:164)

Root cause

getConstructIdsForOutputs maps synth metadata (cdk.tf.json → "//".outputs) onto the real
terraform output -json result with an unchecked lookup:

return { ...acc, [key]: outputs[value] };   // undefined when terraform didn't return it

An output declared in metadata but absent from terraform's result was retained with value undefined.
isObjectEmpty only drops a group where every output is missing, so a partial miss survived. renderNested
then reached that node — isTerraformOutput(undefined) is false, control fell through, and
Object.entries(undefined) threw.

The two nested renderNested frames put the bad value at outputsByConstructId[stack][constructId], depth 1,
which matches. The reporter's network stack mixed user outputs with generated cross-stack-output-* entries
feeding a dependent db stack — a realistic way to get a partial miss (state drift, --skip-synth, an output
renamed or removed between applies).

Why the debug log looked healthy

The logged OutputsByConstructId appeared complete, which initially looked like it contradicted the theory. It
doesn't: those lines log via JSON.stringify, which silently omits undefined-valued keys. The poison map and a
clean map serialize byte-identically — verified — while Object.keys reveals the extra key. The defect was
structurally invisible in the logs.

Why it surfaced in 0.24.0

The defective data path is unchanged since before v0.23 (git diff v0.23.4..v0.24.0 on output.ts is a single
unrelated zod line). What changed is #264, which removed renderInk — whose try/catch used to swallow this
throw as a one-line message and exit(1). Without it the throw escapes unhandled, so a long-latent bug now
produces a stack trace, telemetry and a Sentry report. An exposure regression, not a new defect.

The fix — producer first, with defence in depth

• getConstructIdsForOutputs omits keys whose terraform output is absent, and logs which one was dropped.
  The check is an explicit === undefined, not a truthiness test, so an output whose legitimate value is
  "", 0 or false still renders.
• renderNested and unpackTerraformOutput skip non-object nodes instead of recursing into
  Object.entries. These are deliberately defensive: with the producer fixed there is no known input that
  reaches them, but both walk externally-sourced data and neither should be able to take down the CLI after a
  successful deploy. unpackTerraformOutput is the one that runs first on the --outputs-file path.
• isTerraformOutput excludes null, which previously threw on null.sensitive rather than returning false.
• Rendering the outputs table is now non-fatal in runDeploy. The deploy has already succeeded by that
  point, so a presentation failure is logged instead of failing the deploy — restoring the safety net that
  deleting renderInk removed.

Behaviour note

renderNested now renders nothing for a group whose children all drop, and for a legitimately empty group,
where it previously printed a bare header.

Verification

The new tests were checked against the unfixed code, not just the fixed code: reverting the source changes
while keeping the tests reproduces the exact reported TypeError, with the same nested renderNested frames,
end-to-end through the real runDeploy. Assertions use not.toHaveProperty rather than toEqual, since
toEqual treats a missing key and an undefined value as equal and would pass on the buggy code.

---

Stacked PR

#376 builds on this branch and fixes a separate crash on the --outputs-file path found during review of
this change. It is deliberately split out — the reporter never passed --outputs-file, so none of it is on this
crash's path.

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, no documented behaviour changes
• 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

[sakul-learning]

Review: PR #375 — fix(cli): don't crash after a successful deploy when an output is missing

Verdict: APPROVE ✅

Summary

This PR fixes a crash where the CLI would throw TypeError after a successful terraform apply when a metadata-declared output was absent from terraform output -json (e.g., state drift, renamed output, --skip-synth). The fix adds defense-in-depth at every layer of the output rendering pipeline.

Changes (7 files, +362/−18)

Layer	File	Change
Type guard	packages/@cdktn/cli-core/src/lib/models/terraform.ts	+null guard in isTerraformOutput (typeof null === "object" in JS)
Output mapping	packages/@cdktn/cli-core/src/lib/output.ts	getConstructIdsForOutputs omits absent outputs instead of retaining undefined; unpackTerraformOutput guards null/non-object before Object.entries()
CLI rendering	packages/cdktn-cli/src/bin/cmds/helper/format.ts	renderNested/renderOutputs guard null/non-object values + filter empty-node results
Deploy command	packages/cdktn-cli/src/bin/cmds/ui/deploy.ts	Wraps renderOutputs() in try/catch — render failure is now non-fatal
Tests	output.test.ts, format.test.ts, deploy.test.ts	10 new test cases covering the regression path and edge cases

Configured Checks

Check	Result
pnpm exec nx build cdktn	✅ Pass
pnpm exec nx test cdktn --runInBand	✅ 542 passed, 43 suites, 301 snapshots

Correctness

• isTerraformOutput null guard: Necessary and sufficient — typeof null === "object" is the only falsy value that passes the typeof gate.
• getConstructIdsForOutputs missing-output handling: Correctly omits absent keys. The isObjectEmpty interaction correctly drops groups where all children were omitted.
• unpackTerraformOutput recursion: Null/typeof guard precedes Object.entries(), preventing crashes at any nesting depth.
• renderNested empty-group behavior: Legitimately empty {} groups now print nothing instead of a bare header — safe because getConstructIdsForOutputs already strips empty groups upstream.
• runDeploy try/catch: stream.stop() in finally is guaranteed on all paths (success, render-failure, project-failure). Tests validate: no rejection, callback fires, error logged, stop() called.

One adjacent note (not a blocker)

packages/cdktn-cli/src/bin/cmds/ui/output.ts (the cdktn output command) calls renderOutputs(returnValue) without the same try/catch this PR adds to runDeploy. The same missing-output scenario could crash the output command. Pre-existing and out of scope here — worth a follow-up hardening pass.

Artifact-value

Lean, proportional fix. Every line defends against a real crash. No new abstractions, no speculative code.

Ship it. 🚢

[so0k]

Thanks for the review.

On the adjacent note about cdktn output — good catch, and it is already handled in the stacked #376, which
applies the same treatment to runOutput (it had the identical fire-and-forget onOutputsRetrieved defect as
well as the missing render guard). It is not visible from this PR's diff since #376 sits on top of this branch.

Kept out of this PR deliberately: the reporter of the original crash never passed --outputs-file, so none of
that work is on this crash's path, and #376's change to the exit behaviour deserves its own review.

[jsteinich]

Traced the analysis against main — the root cause and the producer-first fix both hold up. One question and two small notes.

Should the drop be visible?

cdk-terrain/packages/@cdktn/cli-core/src/lib/output.ts

Lines 244 to 246 in 6e8226d

	logger.debug(
	`Output "${value}" (construct id "${key}") declared in stack metadata but absent from terraform output; omitting it.`,
	);

logger is log4js at default level, so this is invisible without CDKTF_LOG_LEVEL=debug. Net effect for the reported scenario: instead of crashing, we print a table quietly missing an output the user declared. Silent is consistent with the existing isObjectEmpty full-miss behaviour, but this is exactly the condition that was invisible in the Sentry report.

Would you consider logger.warn for outputs that aren't cross-stack-output-*? Your call — you've looked at the real data path more closely than I have, and I'll approve either way.

Two smaller things:

1. The non-fatal render guard lands in runDeploy but not runOutput, which has the identical unguarded console.log(renderOutputs(...)). fix(cli): fail cleanly when the --outputs-file write fails #376 covers it — only matters if this ever lands alone.
2. When every group drops, renderOutputs returns "" but runDeploy's Object.keys(outputs).length > 0 gate still passes, so we console.log("") and emit a stray blank line.

[so0k]

Both good catches, and I'll take the suggestion on the first.

Visibility of the drop — yes, logger.warn, split the way you describe.

You've put your finger on the uncomfortable part: the failure mode this PR produces is "quietly missing an
output the user declared", and that is the same class of silence that made the original crash so hard to
diagnose from the Sentry report. Trading a crash for a silent omission is a poor trade for a user-declared
output.

The cross-stack-output-* split is the right seam — those are generated plumbing for stack dependencies, so a
warning per missing one would be noise the user cannot act on. A user-declared output vanishing is actionable:
it means their state and their synthesized metadata disagree. So: logger.warn for user-declared,
logger.debug for cross-stack-output-*.

The stray blank line — real bug, mine.

runDeploy gates on Object.keys(outputs).length > 0, which counts keys whose values were all dropped, so
renderOutputs returns "" and we console.log(""). Introduced by the empty-group change in this PR. Fixing
by gating on the rendered string rather than the key count, with a test.

runOutput — agreed, and covered in #376. This PR is not intended to land alone, but you're right that it
would be a gap if it did.

Both changes incoming; I'll push and re-request.

[so0k]

Pushed b36961754 with both changes.

• logger.warn for user-declared outputs, logger.debug kept for cross-stack-output-*. No shared
  constant for that prefix exists anywhere (it's an inline template literal in
  terraform-stack.ts:535, used once), so this defines a local CROSS_STACK_OUTPUT_PREFIX with a comment
  pointing at the origin rather than importing something that isn't there. Tests cover both branches.
• Stray blank line gone. The gate is now the rendered string rather than the key count.

One judgement call worth flagging, since it's user-visible: when the keys survive but every output under them
drops, this prints No outputs found. rather than nothing — from the user's point of view there is nothing to
show, so it should read the same as the genuinely-empty case.

Note that #376 then changes this case slightly: because the written to line is keyed off write success rather
than render state, --outputs-file with all-dropped outputs prints No outputs found. and the written to
line. Both are defensible; calling it out so the two PRs read consistently.

[jsteinich]

I don't really think something being a cross stack output is all that relevant to whether or not a message is logged.
A missing reference could cause incorrect incorrect plans (though that might just result in a Terraform error), or it could be completely benign. The context of what the user is doing matters.
The same seems true for user defined outputs.

[jsteinich]

Seems like omitting this check is going to generate a warning that could easily be avoided.

[jsteinich]

If renderFailed, is this actually true?