feat: surface validation violations and CFN resources in LSP - #1592
Conversation
ShadowCat567
left a comment
There was a problem hiding this comment.
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
| ? buildTree(rawTree, stackIndex, onWarn) | ||
| : []; | ||
|
|
||
| let violations: PolicyValidationReportJson | undefined; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pretty sure that online validations are not part of this report.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Does cdk validate rewrite the files in cdk.out ?
There was a problem hiding this comment.
it does call cdk synth, which does rewrite them.
…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.
41b998a to
2f91341
Compare
- 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.
ShadowCat567
left a comment
There was a problem hiding this comment.
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
| * metadata to produce a ConstructNode tree where every CFN resource carries | ||
| * its logicalId, CFN type, and source location. | ||
| * | ||
| * Supports: |
There was a problem hiding this comment.
is there anything you know of that we don't support with this?
There was a problem hiding this comment.
Not that I know of, mostly added this comment for clarity of what I had tested and designed for.
| cache: SourceMapCache, | ||
| onWarn?: WarnFn, | ||
| ): SourceLocation | undefined { | ||
| const frames = pickCreationFrames(metadataEntries); |
There was a problem hiding this comment.
what are frames in this context?
There was a problem hiding this comment.
lines of the creation stack trace. made this better documented
|
|
||
| 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 |
There was a problem hiding this comment.
Do we stay on a certain version of aws-cdk-lib/cloud-assembly schema or does this update with them?
There was a problem hiding this comment.
there's no version pin. The package depends on @aws-cdk/cloud-assembly-schema with an any-future version range, so when the schema bumps we pick it up automatically.
The only place a version literal appears is ASSEMBLY_SCHEMA_VERSION = '53.0.0' in builders.ts. That's the version we write into test fixtures, not a version we read against.
There was a problem hiding this comment.
I think I need more context about what these fixtures are and why we need them before I can properly review them
There was a problem hiding this comment.
The reader consumes a real cdk.out/, so tests need realistic cdk.out/ directories. I do that programmatically rather than checking in pre-synthesized cdk.out/s for two reasons: (1) tests read as a typed spec of the shape under test (buildNestedAssembly, buildNonTypeScriptAssembly, etc.) rather than an opaque directory, and (2) checked-in cdk.out/s drift every time aws-cdk-lib or the cloud-assembly schema bumps, and the diffs are over generated JSON.
The one exception is source-maps/: real tsc output, because a builder can fabricate stack traces but can't fabricate a .js.map tsc would accept.
I updated the readme... does this help? happy to chat if it needs more explanation/justfiication
| 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 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Shouldn't this code live in @aws-cdk/cloud-assembly-api ?
There was a problem hiding this comment.
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.
| ? buildTree(rawTree, stackIndex, onWarn) | ||
| : []; | ||
|
|
||
| let violations: PolicyValidationReportJson | undefined; |
There was a problem hiding this comment.
Pretty sure that online validations are not part of this report.
| /** | ||
| * Mirrors toolkit-lib's findCreationStackTrace preference: prefer the | ||
| * aws:cdk:logicalId.trace, fall back to aws:cdk:creationStack.data. | ||
| */ |
There was a problem hiding this comment.
This shouldn't just mirror, it should literally be the same code path.
There was a problem hiding this comment.
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?
| * 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. | ||
| */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This was an initial iteration, but the more I think about it I should just update this now. Fixed. File-known-but-no-line violations now anchor at the top of the file rather than dropping (see the comment below). Truly location-less violations (no file at all) can't become LSP diagnostics, so they're dropped and surfaced in the warning log. That's intentional for now; lmk if you think there's some good project-level default instead you'd prefer over dropping.
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.
loadTraceMap now honors the //# sourceMappingURL directive via convert-source-map (inline data: URIs and external maps under any filename), instead of assuming a sibling <js>.map. Sources are resolved through the map URL, so sourceRoot and maps that live in a different directory than the .js resolve correctly. Adds convert-source-map dep + tests (inline, renamed-external, sourceRoot). (rix0rrr)
| * input location unchanged when it isn't a .js file or has no usable map. | ||
| */ | ||
| private mapJsToOriginalSource(loc: SourceLocation): SourceLocation { | ||
| if (loc.file.endsWith('.ts') || loc.file.endsWith('.tsx') || !loc.file.endsWith('.js')) return loc; |
There was a problem hiding this comment.
This function assumes that the file extension is either ".ts", ".tsx" or ".js". But there is nothing guaranteeing this. If we add Python stack traces along with the JS ones, for example, this will start receiving file names ending in ".py". We need to filter out everything that is not in this allow-list before calling the function.
There was a problem hiding this comment.
good catch for the incoming jsii changes, just pushed an update, let me know what you tjink
Manifest.loadValidationReport runs an assembly-version-compat check that throws on reports lacking a `version` field, which older aws-cdk-lib emits (legacy formatLegacyJson shape). Read the report as data instead; the explorer only consumes pluginReports, which are version-independent across producer versions. - withMalformedValidationReport now writes unparseable JSON so the error path stays genuinely exercised (an invalid-semver version now parses fine) - add withVersionlessValidationReport builder + regression test
resolveFrames now drops creation-stack frames whose file is outside the supported allow-list (.ts/.tsx/.js) rather than passing them through. Once jsii propagates host-language stack traces (aws/jsii#5153), frames like my_stack.py would otherwise parse and leak through as unresolved locations. The allow-list is applied before mapJsToOriginalSource, whose extension guard is simplified accordingly. - add a regression test for a .py frame yielding no location
| '@jridgewell/trace-mapping@^0.3', | ||
| 'convert-source-map@^2', |
There was a problem hiding this comment.
Every new dependency is a liability.
What's the research thas has gone into picking these specific ones?
There was a problem hiding this comment.
Here's where I am in that research. The two libs do different jobs, convert-source-map extracts the map from the .js, trace-mapping decodes it to the original .ts position. Right now, both of these deps are already in our tree, but only as dev-indirect deps which both resolve in main's yarn.lock today via jest/vitest/babel.
For what it's worth on the safety signals both are MIT, have 0 advisories in OSV across all versions, convert-source-map has zero dependencies, and trace-mapping only pulls its author's own resolve-uri/sourcemap-codec and is maintained by a Babel core member (it's what Babel/Jest/Vite use). Neither have a dependabot config, and 'trace-mapping' doesn't have a security policy (but 'convert-source-map' does). Also, convert-source-map isn't actively maintained, but it looks to be feature complete.
The extractor is basically two regexes + base64 + JSON.parse, so I can inline the slice I use and drop that dep. The one thing I actually need from it is their sourceMappingURL regexes, and I definitely don't think I can lift those verbatim without attribution. What's our convention there? Is there a method for attribution, or would you rather I just keep the dep?
For the decoder, looking for alternatives, I checked native Node, module.findSourceMap() only works for files the process has actually loaded with --enable-source-maps, and we read emitted .js out of cdk.out that we never execute, so it can't help with extraction. The SourceMap class could do the decode though. The catch is it returns the raw source path without applying sourceRoot or resolving relative sources, so I'd have to do that myself. Most of the edge cases trace-mapping handles don't (shouldn't :P) really show up in cdk.out output, those are web-bundler artifacts. So native decode is viable also; just depends on if its worth owning the edge cases.
I'm not sure what our bar is for a runtime dep, so would really appreciate your input, given this info. Lmk what you think.
There was a problem hiding this comment.
If it's maintained by someone whose full-time job is JavaScript things, and they don't have too many transitive dependencies (and are somewhat popular?) I'm fine with adding these. Thanks for the digging.
| @@ -0,0 +1,81 @@ | |||
| import * as fs from 'fs'; | |||
There was a problem hiding this comment.
Didn't you say you were moving this code?
There was a problem hiding this comment.
Yep, the generic tree machinery moved, so buildConstructTree, ConstructIndex, and ConstructTreeNode are in @aws-cdk/cloud-assembly-api now, and this file imports them. What's left here is the explorer-specific layer, so readAssembly, the ConstructNode type that adds sourceLocation, and loadViolations. Those stay because they pull in toolkit-lib (findCreationStackTrace) and the source-map resolver. cloud-assembly-api can't depend on toolkit-lib, since that would be circular. So cloud-assembly-api builds the generic tree and the explorer decorates it with source locations via a callback
| const parsed = parseFrame(frame); | ||
| if (!parsed) continue; | ||
| if (!isSupportedSourceFile(parsed.file)) return undefined; | ||
| return this.mapJsToOriginalSource(parsed); |
There was a problem hiding this comment.
for source tracing with non-TS apps would we need to make more methods like this or is there a way we can extend this logic to work for any language? (we might not know the answer to this until source tracing with non-TS gets implemented)
There was a problem hiding this comment.
we shouldn't need a separate resolver per language.... source maps are a TSthing, so a Python/Java creation frame already points at the original source, nothing to decode.
So extending is mostly (1) add the extension to the allow-list and (2) parse that language's frame format, depending on how that stabilizes upstream.
Per review feedback: the class specifically resolves JS source maps back to original positions, so the narrower name is clearer.
Derive ASSEMBLY_SCHEMA_VERSION from Manifest.version() so it tracks the installed schema and can't drift, and pull the repeated constructInfo and tree.json version literals into CONSTRUCT_INFO_VERSION / TREE_SCHEMA_VERSION.
rix0rrr
left a comment
There was a problem hiding this comment.
Provisional approval! That means I expect you to the address the last few comments, but I trust you to fix those and I don't need to see the PR again before you merge.
Revert the Manifest.version() derivation. Per review (rix0rrr): a fixture frozen at a known schema revision is a deliberate forward-compat test that the reader can still load older assemblies. Deriving the version would make the manifest claim the latest revision while its contents stay frozen.
Builds on #1592, which surfaced display-only CodeLens entries for the CFN resources each construct produces. This PR makes them **clickable**. Selecting a lens jumps to the resource's definition in the synthesized CloudFormation template. Features: - **Clickable navigation** — each lens carries an `openResource` command that opens the resource's template file at its logical-ID line. - **Multi-resource picker** — constructs producing several resources show a QuickPick; single-resource constructs open directly. - **Positional `templateFile` resolution (cloud-assembly-api)** — `buildConstructTree` threads the owning template through the tree, switching at NestedStack boundaries. - **Clearer titles** — `Creates AWS::S3::Bucket`, or `Creates 3 resources: AWS::S3::Bucket, AWS::S3::BucketPolicy, AWS::KMS::Key`. ### 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 --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Merges the`feat/cdk-lsp' branch into `main`. The change is additive and introduces no behavior change to existing CLI commands. - New `@aws-cdk/cdk-explorer` package containing the Language Server under `lib/lsp` (server, diagnostics, CodeLens, template locator, position mapping). - Extends `@aws-cdk/cloud-assembly-api` with two parsing modules consumed by the server: `construct-tree.ts` (builds the construct tree from a cloud assembly) and `template-ranges.ts` (resolves a logical ID or property to its byte range in the template). Capabilities (folds in aws#1559, aws#1593, aws#1592, aws#1617, aws#1624, aws#1630, aws#1631, aws#1662, aws#1634, aws#1674): - Diagnostics: surfaces synth errors and policy-validation violations in the editor, mapped back to the source. - Surfaces CFN resources and adds CodeLens navigation from a construct to its template resource. - Navigation between construct source and the synthesized template in both directions. - Live refresh: diagnostics and CodeLens update when `cdk.out` changes. - Reads are constrained to the project directory, and template reads run off the LSP event loop. This PR is the server and parsing foundation. It does not add a shipped CLI command or the web explorer. ### 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 --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com>



cdk.out/'stree.jsonwith each stack's manifest metadata into aConstructNodetree carryinglogicalId, CFN type, and source location.Diagnostics anchored to the construct's TypeScript source line, with rule-level severity.CodeLensentries above each construct creation site summarising the CFN resources it produces..tsand.js(with sibling.js.map); non-TS apps degrade gracefully (no crash, no source-linked features).Fixes #
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license