feat: surface validation violations and CFN resources in LSP by megha-narayanan · Pull Request #1592 · aws/aws-cdk-cli

megha-narayanan · 2026-06-03T19:35:05Z

Adds an assembly reader that joins cdk.out/'s tree.json with each stack's manifest metadata into a ConstructNode tree carrying logicalId, CFN type, and source location.
LSP publishes CDK validation violations as Diagnostics anchored to the construct's TypeScript source line, with rule-level severity.
LSP serves CodeLens entries above each construct creation site summarising the CFN resources it produces.
Source resolution covers .ts and .js (with sibling .js.map); non-TS apps degrade gracefully (no crash, no source-linked features).

Fixes #

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

ShadowCat567

Initial comments, will come back and do more of a review of the logic for the codelens + diagnostics.
In general for LSP features, it would be helpful to have some screenshots of what you are implementing looks like in the IDE

ShadowCat567 · 2026-06-03T20:10:40Z

+      ? buildTree(rawTree, stackIndex, onWarn)
+      : [];
+
+    let violations: PolicyValidationReportJson | undefined;


Can you check whether online validations appear in this report?
Since I think it might be possible that a customer runs cdk validate themselves and online validations appear in the report with offline validations.

Yep, I think (in the actual version of validate) they do. the schema doesn't distinguish between online and offline violations, so we can't really filter them out. My thought would be that this is a good thing though, right? If we have violations detected, we should surface them the same way. ofc, if tehres no source location thats a separate issue and they get filtered out anyway.

Pretty sure that online validations are not part of this report.

depends. if running cdk synth, online validation is not a part of the report. if running cdk validate --online, then it will be a part of the report.

the plugin name will be CloudFormation -- see https://github.com/aws/aws-cdk-cli/pull/1539/changes#diff-efa522e30d6c1c6a99aa8a5da647de2173348e9dd3c571bc2f37ec0916adbc33R758-R762 -- so we can filter those out if needed. or just run validate --offline

…n LSP Joins cdk.out/'s tree.json with manifest metadata into a ConstructNode tree. LSP publishes validation violations as line-anchored Diagnostics and CFN resources as CodeLens above each construct's creation site. Resolves .ts and .js (via sibling .js.map); non-TS apps degrade gracefully. Tests use programmatic fixture builders covering flat, Stage-based, and NestedStack assemblies.

- assembly-reader: convert RawTreeNode comment to JSDoc so it's clearly attached to the interface (not an orphan above it). - diagnostics: trim the Number.MAX_VALUE end-of-line comment to one line; the LSP-spec details aren't necessary at this point. - source-resolver: drop the test-only `_clearTraceMapCache` export. The source-map cache is now an explicit `SourceMapCache` parameter created per `readAssembly` call, with `createSourceMapCache()` as the factory. Tests construct a fresh cache in `beforeEach`. Production has one cache per assembly read, so repeated `.js.map` parses still amortise. - assembly-reader.test: replace the `if (result.status !== 'success') throw new Error(...)` narrowing trick (11 sites) with an `expectSuccess(result)` helper that asserts via `expect(...).toBe('success')` and returns the typed `data`. No more thrown errors in test bodies.

megha-narayanan · 2026-06-04T20:18:39Z

Ugly but functional surfacing of resources with source resolution

megha-narayanan · 2026-06-04T20:25:22Z

And violations:

ShadowCat567

The biggest question I have is related to the fixtures, I don't think it's a pattern that is used elsewhere in this repo and I would like a bit more explanation in the readme about what their deal is

ShadowCat567 · 2026-06-04T19:23:04Z

+ * metadata to produce a ConstructNode tree where every CFN resource carries
+ * its logicalId, CFN type, and source location.
+ *
+ * Supports:


is there anything you know of that we don't support with this?

Not that I know of, mostly added this comment for clarity of what I had tested and designed for.

ShadowCat567 · 2026-06-04T19:25:54Z

+    try {
+      violations = loadViolations(assemblyDir);
+    } catch (err) {
+      violationsError = err instanceof Error ? err.message : String(err);


errors are usually Objects, JSON.stringify() would probably give you nicer looking output than String() - similar comment with the other places you are using String() instead of JSON.stringify()

Just assume that err is always an Error, and reject any code that throws anything else during code review.

ShadowCat567 · 2026-06-04T20:10:10Z

+  cache: SourceMapCache,
+  onWarn?: WarnFn,
+): SourceLocation | undefined {
+  const frames = pickCreationFrames(metadataEntries);


what are frames in this context?

lines of the creation stack trace. made this better documented

ShadowCat567 · 2026-06-04T20:18:03Z

+    range,
+    severity: severityFor(violation.severity),
+    code: violation.ruleName,
+    source: 'CDK Synth',


Why do all of our violations have a source of CDK synth could this instead be used to surface which plugin it came from?

ShadowCat567 · 2026-06-04T20:23:20Z

+
+Most tests build their `cdk.out/` programmatically via `builders.ts` —
+keeps test intent in TypeScript and avoids drift when aws-cdk-lib or the
+cloud-assembly schema upgrades. `builders.test.ts` sanity-checks each


Do we stay on a certain version of aws-cdk-lib/cloud-assembly schema or does this update with them?

ShadowCat567 · 2026-06-04T20:23:55Z

I think I need more context about what these fixtures are and why we need them before I can properly review them

ShadowCat567 · 2026-06-04T20:30:57Z

And violations:

:O Do the squiggles change color based on severity?

otaviomacedo · 2026-06-05T08:28:02Z

+import { createSourceMapCache, resolveSourceLocation, type SourceLocation, type SourceMapCache, type WarnFn } from './source-resolver';
+
+/** A construct from tree.json plus the CFN metadata the LSP surfaces. */
+export interface ConstructNode {


Eventually we'll also need to store information about properties and their assignment locations. So, just think about how you're going to model it then, and if the current model is extensible for that. For example, you could add a new attribute to this interface called something like properties. Or you could decide to have a tree in which the nodes may be constructs or properties (and properties are children of constructs).

Yep, i think the model is extensible for this. I'd add an optional properties?: PropertyNode[] to the node (PropertyNode { name; sourceLocation }) rather than making properties tree children, so the construct index/iteration and CodeLens stay construct-only. The data's already available PROPERTY_ASSIGNMENT metadata ({ propertyName, stackTrace }) flows through the same buildConstructTree decorate callback that resolves creation-site locations, so resolving an assignment location reuses the exact source-map machinery. Planning to land the field + populate it as a follow-up to keep this PR scoped.

rix0rrr · 2026-06-05T08:48:14Z

Shouldn't this code live in @aws-cdk/cloud-assembly-api ?

Done. Moved the tree builder into @aws-cdk/cloud-assembly-api as buildConstructTree(assembly, decorate) it owns the tree.json parse + stack-metadata join and produces a generic ConstructTreeNode. The explorer passes a decorate callback that adds sourceLocation; that stays here since it needs trace-mapping (dependency). I also moved ConstructIndex there so it's reusable, which is why it's generic over the node type (ConstructIndex); the explorer uses ConstructIndex to keep its sourceLocation. If you'd rather the index stay specific to teh explorer, easy to change. lmk if this looks good, it adds a small public API surface.

rix0rrr · 2026-06-05T08:50:31Z

+      ? buildTree(rawTree, stackIndex, onWarn)
+      : [];
+
+    let violations: PolicyValidationReportJson | undefined;


Pretty sure that online validations are not part of this report.

rix0rrr · 2026-06-05T08:51:05Z

+    try {
+      violations = loadViolations(assemblyDir);
+    } catch (err) {
+      violationsError = err instanceof Error ? err.message : String(err);


Just assume that err is always an Error, and reject any code that throws anything else during code review.

rix0rrr · 2026-06-05T08:52:38Z

+ *    nested-stack constructs into the PARENT stack's artifact metadata,
+ *    keyed by full construct path.
+ */
+export function readAssembly(assemblyDir: string, onWarn?: WarnFn): AssemblyReadResult {


Why not return warnings on the successful result?

rix0rrr · 2026-06-05T08:54:23Z

+/**
+ * Mirrors toolkit-lib's findCreationStackTrace preference: prefer the
+ * aws:cdk:logicalId.trace, fall back to aws:cdk:creationStack.data.
+ */


This shouldn't just mirror, it should literally be the same code path.

you're totally right. I was trying to evoid messing with APIs, but I need to do it at some point. I exported findCreationStackTrace from toolkit-lib's source-tracing so this can be the literal same code path; it takes (stack, constructPath). While there I also exported findMutationStackTraces for the property-assignment locations otavio raised. Source-map decode stays in the explorer. ok to add these two to toolkit-lib's public API?

rix0rrr · 2026-06-05T09:12:44Z

+ * Convert a validation report into LSP diagnostics keyed by file URI.
+ * Violations whose construct path is unknown, has no source location, or
+ * points outside TypeScript are dropped (with a reason) rather than thrown.
+ */


I thought we were going to represent warnings without a source location in some default location. Is this an initial iteration, or is this the final plan?

rix0rrr · 2026-06-05T09:13:44Z

+  const loc = node.sourceLocation;
+  if (!loc) return { error: 'node has no sourceLocation (non-TS or framework-only trace)' };
+
+  if (!loc.file.endsWith('.ts') && !loc.file.endsWith('.tsx')) {


I'd rather have "limiting to TypeScript for now" be a type of processing we do consciously do at the start, rather than something in the middle of different kinds of error recovery code.

rix0rrr · 2026-06-05T09:15:00Z

+  if (!constructPath) return { error: 'empty constructPath' };
+
+  const node = nodesByPath.get(constructPath);
+  if (!node) return { error: 'no tree node for constructPath' };


What are these errors for?

If we show them to users, I would like them to look like messages that are appropriate to show to a user (could not find this node in tree.json).

If we're not going to show this to a user, then they could be error codes or we don't have this field at all.

rix0rrr · 2026-06-05T09:15:36Z

+    return { error: `non-TypeScript source: ${loc.file}` };
+  }
+  if (loc.line < 1 || loc.column < 1) {
+    return { error: `invalid line/column: ${loc.line}:${loc.column}` };


But we could still relate it to a file? Can't we stick it at line 1 of this file then?

rix0rrr · 2026-06-05T09:20:30Z

+    readAssembly: options.readAssembly,
+    logger: connection.console,
+    onPublishDiagnostics: (uri, diagnostics) => {
+      void connection.sendDiagnostics({ uri, diagnostics });


What's the void here, and why is that okay?

Is that a silenced "you shall await this" linter error?

Addresses PR aws#1592 review feedback: - Move the tree.json + stack-metadata join (buildConstructTree) and ConstructIndex into @aws-cdk/cloud-assembly-api as generic, reusable cloud-assembly tooling. cdk-explorer passes a decorate callback that adds sourceLocation (stays here; needs trace-mapping). - Introduce ConstructIndex (a keyed Map over the tree), removing the duplicated walk() in tree-utils and codelens. - codeLensesForFile now takes a ConstructIndex and uses filter/map + a groupBy combinator.

…SOURCE_TYPE_ATTRIBUTE Make two assembly-format constants canonical in cloud-assembly-schema instead of hardcoded in consumers: - VALIDATION_REPORT_FILE ('validation-report.json') in manifest.ts, next to loadValidationReport. - CFN_RESOURCE_TYPE_ATTRIBUTE ('aws:cdk:cloudformation:type') tree-node attribute in metadata-schema.ts. cloud-assembly-api (construct-tree) and cdk-explorer (assembly-reader, test fixture) now consume them from schema.

…nsts) - source-resolver: rename ambiguous 'frames' to 'creationStackFrames' (ShadowCat567). - construct-tree: move CDK_INTERNAL_IDS to the top beneath imports (ShadowCat567).

The builder moved here from cdk-explorer but only ConstructIndex was tested, dropping function coverage below threshold. Add buildConstructTree tests (tree+metadata join, internal-node filtering, decorate callback, empty/no-tree case).

… frame selection Export findCreationStackTrace + findMutationStackTraces from toolkit-lib's source-tracing, and have cdk-explorer call findCreationStackTrace instead of its duplicate pickCreationFrames (now deleted). buildConstructTree's decorate callback now provides (fields, stack, constructPath) so the explorer can call the toolkit-lib tracer; source-resolver keeps only the frame->location source-map decode (resolveFramesToLocation).

…tree artifact buildConstructTree now resolves the tree file via assembly.tree().file instead of assuming 'tree.json', so it honors whatever filename the manifest declares (and returns [] when there is no tree artifact).

The functions threshold was temporarily lowered to 50 in the first (skeleton) PR. The package has grown and now sits at 81% function coverage, so restore it to 80 to match statements/branches/lines.

megha-narayanan temporarily deployed to automation June 3, 2026 19:35 — with GitHub Actions Inactive

github-actions Bot added the p2 label Jun 3, 2026

megha-narayanan requested a deployment to integ-approval June 3, 2026 19:35 — with GitHub Actions Waiting

aws-cdk-automation requested a review from a team June 3, 2026 19:35

megha-narayanan changed the title ~~feat(cdk-explorer): surface validation violations and CFN resources in LSP~~ feat: surface validation violations and CFN resources in LSP Jun 3, 2026

aws-cdk-automation requested a deployment to integ-approval June 3, 2026 19:43 — with GitHub Actions Waiting

megha-narayanan marked this pull request as ready for review June 3, 2026 19:58

ShadowCat567 reviewed Jun 3, 2026

View reviewed changes

megha-narayanan force-pushed the feat/explorer-assembly-reader branch from 41b998a to 2f91341 Compare June 4, 2026 18:27

megha-narayanan changed the base branch from feat/cdk-lsp-explorer to feat/cdk-lsp June 4, 2026 18:27

megha-narayanan requested a deployment to integ-approval June 4, 2026 18:27 — with GitHub Actions Waiting

megha-narayanan requested a deployment to integ-approval June 4, 2026 20:07 — with GitHub Actions Waiting

megha-narayanan requested a review from ShadowCat567 June 4, 2026 20:19

ShadowCat567 reviewed Jun 4, 2026

View reviewed changes

otaviomacedo reviewed Jun 5, 2026

View reviewed changes

rix0rrr reviewed Jun 5, 2026

View reviewed changes

megha-narayanan requested a deployment to integ-approval June 8, 2026 16:39 — with GitHub Actions Waiting

megha-narayanan added 6 commits June 8, 2026 13:10

refactor: address review nits (rename creation-stack frames, group co…

6026631

…nsts) - source-resolver: rename ambiguous 'frames' to 'creationStackFrames' (ShadowCat567). - construct-tree: move CDK_INTERNAL_IDS to the top beneath imports (ShadowCat567).

chore(cdk-explorer): restore functions coverage threshold to 80

1dee45c

The functions threshold was temporarily lowered to 50 in the first (skeleton) PR. The package has grown and now sits at 81% function coverage, so restore it to 80 to match statements/branches/lines.

megha-narayanan requested a deployment to integ-approval June 8, 2026 17:49 — with GitHub Actions Waiting

Conversation

megha-narayanan commented Jun 3, 2026

Checklist

Uh oh!

ShadowCat567 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

megha-narayanan commented Jun 4, 2026

Uh oh!

megha-narayanan commented Jun 4, 2026

Uh oh!

ShadowCat567 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megha-narayanan Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShadowCat567 commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megha-narayanan Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megha-narayanan Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

megha-narayanan Jun 8, 2026 •

edited

Loading

megha-narayanan Jun 8, 2026 •

edited

Loading

megha-narayanan Jun 8, 2026 •

edited

Loading