feat(toolkit): add cdk lsp - #8828
Conversation
…K CLI Discover the user's `cdk` CLI (aws.cdk.cliPath setting, else workspace node_modules, else shell PATH) and start `cdk lsp` over stdio for the CDK app in the workspace. Gated on cdk >= 2.1132.0, the first release carrying the lsp command. Reuse detectCdkProjects for the activation gate and resolveEnv for a login-shell PATH so the server's synth subprocess resolves node/java. Run a single client per window (aws.cdk.appDir selects the app when several are present), since each client re-registers the server's executeCommand ids and a duplicate registration throws. Emit cdk_startLanguageServer telemetry on success and failure.
Overlapping restart triggers (activation, config/folder changes, a new cdk.json) could run two starts concurrently and leak a client whose reference was overwritten. Serialize restarts and coalesce bursts into one trailing restart. Add the changelog entry for the feature.
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
|
✅ I finished the code review, and didn't find any security or code quality issues. |
rix0rrr
left a comment
rix0rrr
left a comment
There was a problem hiding this comment.
I have some trivial questions on details of the subcommand handling, but overall I support this change. Looks good and I like the edge case handling of file changes that is done.
Provisionally approved from my end
| const merged = await mergeResolvedShellPath(process.env) | ||
| const env: NodeJS.ProcessEnv = { ...merged } | ||
|
|
||
| const resolved = await getResolvedShellEnv(process.env) |
There was a problem hiding this comment.
Why is taking this environment wholesale not the right solution?
There was a problem hiding this comment.
Fair point, switched to taking the resolved shell env instead of cherry-picking JAVA_HOME.
| /** `which`-style scan of env.PATH for an executable. */ | ||
| async function findOnPath(name: string, env: NodeJS.ProcessEnv): Promise<string | undefined> { | ||
| const rawPath = env.PATH ?? process.env.PATH ?? '' | ||
| const exts = process.platform === 'win32' ? ['.cmd', '.exe', '.bat', ''] : [''] |
There was a problem hiding this comment.
This is a bit inconsistent. If you search for both .bat and .cmd here, I feel you should do the same for findNodeModules
There was a problem hiding this comment.
node_modules is npm-controlled and npm only ever writes a .cmd shim there (plus a .ps1 and an extensionless POSIX shell script), and .cmd is the only one Node can spawn on Windows. So cdk.cmd is the sole valid candidate, and looking for .bat/.exe there would be dead code.
findOnPath scans an open-ended PATH where a cdk could come from other installers, so it casts a wider net. The one thing that was genuinely wrong is that I had .bat in that list, and npm never produces a cdk.bat, so I dropped it. The PATH list is now .cmd, .exe, and extensionless, which covers the npm shim, a native binary, and a POSIX cdk respectively.
Both lists now carry a one-line comment explaining the difference so it doesn't look like an oversight.
| */ | ||
| export async function probeCdkVersion(command: string, env: NodeJS.ProcessEnv): Promise<string | undefined> { | ||
| try { | ||
| const proc = new ChildProcess(command, ['--version'], { spawnOptions: { env }, collect: true }) |
There was a problem hiding this comment.
Will this work on Windows if the target is a .cmd file? (That needs the shell)
There was a problem hiding this comment.
For this call it's fine sinceprobeCdkVersion goes through our ChildProcess wrapper, which spawns via cross-spawn, so a .cmd target on Windows is handled. But the actual server start in client.ts uses vscode-languageclient's Executable, and I realized that path does a plain child_process.spawn with no shell, so a cdk.cmd command would fail to launch on Windows even though the probe passed. Just fixed at the source shell: true on Windows for the Executable, with the command path quoted so a path with spaces still parses.
- buildCdkSpawnEnv: take the resolved shell env; host env and unioned PATH win collisions, generalizing the JAVA_HOME gap-fill to all shell vars - findOnPath: drop .bat (npm never produces one); comment why the PATH and node_modules extension lists differ - lsp client: spawn the cdk .cmd through a shell on Windows with a quoted command path, since vscode-languageclient uses raw child_process.spawn
2 participants
The CDK CLI now ships a language server (
cdk lsp, added in aws-cdk 2.1132.0) that links construct source, the synthesized template, and policy violations. This PR startscdk lspfrom the user's own CDK CLI as a stdio language client for the CDK app in the workspace.Design:
aws.cdk.cliPathsetting, workspacenode_modules/.bin/cdk, then PATH. Local first, so the server matches the version the project synths with.cdk lsp). Older or missing CLIs get a dismissible prompt instead of a broken start.detectCdkProjectsso nothing spawns outside a CDK workspace, and resolve a login-shell PATH viaresolveEnvso the server'ssynth subprocess finds node and java. Credentials are not injected.
aws.cdk.appDirselects the app when a workspace has more than one. A single client avoids the duplicateexecuteCommandregistration that multiple clients would hit.feature/xbranches will not be squash-merged at release time.