fix(toolkit-lib): NoStack masks the real error when a new stack fails to deploy

[svozza]

Relates to #1802

First of two PRs, split per review feedback on #1803. This is the error-masking half; the stale DescribeStacks read follows separately.

The bug

monitorDeployment passed finalState.wrapped to the diagnoser from inside its catch block. finalState is still the pre-deploy lookup at that point, which holds no stack when the deployment was creating one from scratch — so the wrapped getter threw NoStack. Because that happened while evaluating an argument, it replaced the deployment error being reported:

NoStack: CloudFormationStack object does not hold a stack

Every failed deployment of a new stack was affected, not only the ones from the stale-read bug in #1802. A plain resource failure that rolled the stack back reported NoStack instead of naming the resource, and the diagnoser's CloudTrail enrichment was discarded.

What changed

Diagnosis is an enrichment of the deployment error — it can only ever replace that error with something that says more. The old code broke that in two ways, and the guard for a third was missing. It now lives in one helper that holds to the invariant:

} catch (e: any) {
  // Deployment errors get replaced by a diagnosis of the underlying resource failures, which says more.
  // Any other error, and any failure to diagnose, leaves `e` to propagate as it is.
  if (ToolkitError.isDeploymentError(e)) {
    await this.diagnoseDeploymentFailure(stackArn, monitor.errors);
  }

  throw e;
}

• Describe the stack that was actually deployed. The pre-deploy description is absent when creating and stale when updating, and the diagnoser reads the status off it.
• A failed describe returns without throwing, leaving the original error to propagate rather than replacing it with something less useful. This answers the "but this can also fail... what then?" comment.
• The lookup failure is logged at debug, matching stack-diagnoser.ts:163 — a throttled or access-denied diagnosis was previously invisible even under -v.

Two consequences worth calling out, both of which shrink the surface rather than grow it:

• finalState's initialiser is now provably dead — the try block either assigns or throws, and the catch always rethrows — so the mutable local that meant two different things at two points in the method no longer starts life holding a pre-deploy value. That's what made this bug representable in the first place.
• With the helper describing the stack directly rather than through CloudFormationStack.lookup(...).wrapped, the wrapped getter has no callers left anywhere in the repo, so it's removed. The class whose throwing getter caused this no longer has one.

Testing

New deploy-stack-error-surfacing.test.ts, 6 cases: a failing create via change-set and direct, each with rollback on and off; a failing update of an existing stack (guards against a fix that only works when the stack is missing); and a describe that fails mid-diagnosis, asserting the original ROLLBACK_COMPLETE error survives and the swallowed failure is still visible at debug level.

Written before the fix — 5 of the 6 failed at stack-helpers.ts:75 via the catch block, and the existing-stack case passed both before and after. Each subsequent cleanup was verified load-bearing by reverting it and confirming a specific test failed.

Full toolkit-lib suite passes (1905 tests). No integ test: no new resource types or cross-service interactions.

Follow-ups, deliberately not here

• waitForStackDeploy already holds the settled CloudFormationStack from stabilizeStack and discards it when throwing, so this helper re-describes the same stack. Worth fixing — the CFN client is configured with 7 retries capped at 15s, so on a throttled account that redundant read can add up to ~60s per failed stack, on precisely the path where throttling is the likely cause. It needs a cfn-api.ts signature change, so it's out of scope for this PR.
• Moving diagnosis into CloudFormationStackDiagnoser (so callers never hold a raw Stack) needs Diagnosis.throwOnError() split into throwOnError/throwOnProblem first. Otherwise a diagnoser-owned lookup failure surfaces as ErrorDiagnosisFailed and masks the real error — this same bug class through a different door.

Checklist

• This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed
  • Release notes for the new version:

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.35%. Comparing base (5498a1a) to head (b8d4ee0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1845   +/-   ##
=======================================
  Coverage   90.35%   90.35%           
=======================================
  Files          80       80           
  Lines       12159    12159           
  Branches     1727     1727           
=======================================
  Hits        10986    10986           
  Misses       1139     1139           
  Partials       34       34           

Flag	Coverage Δ
suite.unit	90.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.