Repository navigation
refactor(cdk-explorer): rebuild the core on the assembly index - #2057
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
| for (const vc of violation.violatingConstructs ?? []) { | ||
| const traced = index.traceViolation(vc); | ||
| for (const loc of traced.sourceLocations) { | ||
| yield loc.file; |
mrgrain
left a comment
mrgrain
left a comment
There was a problem hiding this comment.
minor stuff, mostly good
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR is a large internal refactor of the cdk-explorer LSP server, rebuilding its loading and source-mapping core on top of the shared The one blocking concern is the synth debug-flag change: swapping the explicit 🔴 1 blocking · 🟡 6 recommended · ⚪ 4 optional Files with findings (8)Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| /** Resolve a violation to a presentable anchor, or a reason it can't be shown. */ | ||
| function anchorViolation( | ||
| constructPath: string | undefined, | ||
| index: ConstructIndex<ConstructNode>, | ||
| ): { readonly uri: string; readonly range: Range } | { readonly reason: string } { | ||
| const loc = resolveLocation(constructPath, index); | ||
| if ('reason' in loc) return loc; | ||
| violation: ViolatingConstructJson, | ||
| index: AssemblyIndexWithSourceLocations, | ||
| ): Result<{ readonly uri: string; readonly range: Range }[]> { | ||
| const traced = index.traceViolation(violation); | ||
| return traced.sourceLocations.map((loc) => { | ||
| return { uri: pathToFileURL(index.appDir.absolute(loc.file)).toString(), range: toRange(loc) }; | ||
| }); |
There was a problem hiding this comment.
🟡 Recommended — A violation whose traced sourceLocations is empty is silently discarded — it produces neither a diagnostic nor a dropped entry. anchorViolation returns traced.sourceLocations.map(...), so an empty list yields []; isFailure([]) is false, so the located branch runs with nothing to append and the dropped path is never taken. The doc comment above mapViolationsToDiagnostics still asserts such violations "are dropped (with a reason) rather than thrown," which is now inaccurate, and server.ts logs every dropped entry via log.warn, so a framework-only or unresolved violation now vanishes without any trace to the user.
Suggested change: Treat an empty sourceLocations as a drop with a reason — e.g. have anchorViolation return { reason: 'no source location (framework-only trace or unresolved source)' } when traced.sourceLocations.length === 0, so the existing dropped/log.warn path still surfaces it; or, if the silence is intentional, update the comment to match.
| */ | ||
| export async function retryOnLock(abort: AbortSignal, fn: () => Promise<IReadableCloudAssembly>) { | ||
| let delayMs = 500; | ||
| const deadline = AbortSignal.any([abort, AbortSignal.timeout(60 * 60 * 1000)]); |
There was a problem hiding this comment.
🟡 Recommended — retryOnLock calls AbortSignal.any([...]), which is only available from Node.js 18.17.0 (and 20.3.0), but this package declares engines.node as >= 18.0.0. On a Node 18.0–18.16 runtime the first lock-retry throws TypeError: AbortSignal.any is not a function, and retryOnLock sits on both the primary read path (readAssemblyDirectory) and the synth path, so the explorer would fail to read or synth an assembly there. The declared engine contract and the code's actual requirement disagree.
Suggested change: Confirm the intended minimum Node version. If Node 20 is the real target, raise engines.node accordingly via .projenrc.ts; if 18.0 must be supported, compose the deadline without AbortSignal.any (e.g. a manual setTimeout-driven AbortController plus an abort listener).
| @@ -33,7 +25,7 @@ export class SourceMapResolver { | |||
| * the source maps they point at), which are attacker-influenceable if | |||
| * cdk.out is tampered with, so anything escaping the root is dropped | |||
There was a problem hiding this comment.
⚪ Optional — The SourceMapResolver constructor's JSDoc documents @param projectRoot - Absolute path to the project..., but the constructor's only parameter is index: AssemblyIndexWithSourceLocations. The projectRoot parameter no longer exists on this constructor, so the comment describes a signature that isn't there and misleads the next reader about what the resolver takes.
Suggested change: Replace the @param projectRoot block with one describing index; the attacker-influence rationale is still valuable — re-key it to the actual parameter or move it to the class-level doc.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * LSP range for a source location. Anchors at the resolved line/column, or the | ||
| * top of the file when line/column aren't known. LSP positions are 0-based; | ||
| * sourceLocation is 1-based. The end spans the rest of the line. |
There was a problem hiding this comment.
⚪ Optional — The toRange doc comment still describes column handling ("Anchors at the resolved line/column … LSP positions are 0-based; sourceLocation is 1-based") and a hasLineCol variable, but this change removed column entirely: toRange(loc: { line: number }) always sets character: 0. The comment now describes behavior the function no longer has, which will mislead the next reader into thinking a column is honored.
Suggested change: Drop the "column" language from the comment and state that the anchor is always at column 0 by design, reflecting the deliberate column drop.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| } | ||
| const controller = new AbortController(); | ||
| for (const signal of signals) { | ||
| // If we start aborted, no need to add the event listener. | ||
| if (signal.aborted) { | ||
| controller.abort(); | ||
| break; | ||
| } | ||
| signal.addEventListener('abort', () => controller.abort()); | ||
| } |
There was a problem hiding this comment.
🟡 Recommended — The anyAbortSignal polyfill registers signal.addEventListener('abort', …) on each input signal but never removes the listeners. It is called by retryOnLock on every synth and every assembly refresh, always against the single process-lifetime shutdown signal, so over an active editing session (save → synth, cdk.out change → refresh) these listeners accumulate without bound on one AbortSignal, eventually tripping Node's MaxListenersExceededWarning and holding references for the life of the server. The fresh one-hour AbortSignal.timeout created per retryOnLock call likewise accumulates. This polyfill branch is the real runtime path on Node 18.0–18.16, since AbortSignal.any only landed in 18.17.0 while the repo supports Node >= 18.0.0.
Suggested change: Register the listeners with { once: true } and, when the combined controller aborts, remove any still-attached listeners (track them and clean up in a finally/abort handler); consider creating the one-hour deadline once per server rather than per retryOnLock call.
| this.pidString = `${process.pid}`; | ||
|
|
||
| this.writerFile = path.resolve(this.directory, 'synth.lock'); | ||
| this.writerFile = path.resolve(this.directory, SYNTH_LOCK_FILE); |
There was a problem hiding this comment.
⚪ Optional — This change only extracts the literal 'synth.lock' into a module-private const SYNTH_LOCK_FILE used solely by the existing constructor. It is not exported and nothing in cdk-explorer imports it, so within this PR it is a pure no-op rename in a different package from the one the change is scoped to. It is harmless, but it is an unrelated hunk (AGENTS.md#pr-conventions, "one concern per PR"). Worth confirming it is intended groundwork for a later change in the stack rather than a stray edit; if it has no consumer here, consider dropping it or splitting it out.
Suggested change: Confirm this constant extraction is intended groundwork for a later change in the stack; if it has no consumer in this PR, drop it or split it into its own change.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| export async function runSynth(options: SynthRunnerOptions): Promise<SynthRunResult> { | ||
| const app = readCdkConfig(options.projectDir).app; | ||
| if (app === undefined) return { status: 'unavailable' }; | ||
|
|
||
| let cached; | ||
| let asm; | ||
| try { | ||
| const cx = await options.toolkit.fromCdkApp(app, { | ||
| workingDirectory: options.projectDir, | ||
| const cx = await options.toolkit.fromCdkAppDirectory(options.projectDir, { | ||
| lookups: false, | ||
| // Make jsii forward host-language (Python/Java) source frames; TS resolves via source maps. | ||
| env: { JSII_HOST_STACK_TRACES: '1' }, | ||
| synthOptions: { | ||
| debug: true, | ||
| }, |
There was a problem hiding this comment.
🔴 Blocking — The refactor replaces fromCdkApp(app, { env: { JSII_HOST_STACK_TRACES: '1' } }) with fromCdkAppDirectory(projectDir, { synthOptions: { debug: true } }), relying on debug: true to make the synthesized app emit construct creation-stack traces. Those traces are the sole input the assembly index parses into source locations — the basis of every code lens, hover, go-to-definition, and policy diagnostic this package produces. In toolkit-lib, synthParametersFromSettings gates both CDK_DEBUG and JSII_HOST_STACK_TRACES behind the debugApp setting, never debug; the debug→debugApp expansion lives only in the CLI package. So synthOptions: { debug: true } appears to set neither env var, leaving creationStack empty for every node and the Explorer with nothing to anchor. The accompanying unit test was changed to assert nothing about the environment and runs against a fake toolkit, so it cannot catch this.
Suggested change: Verify against a real Toolkit (not the fake) that debug: true actually yields creation stacks; if it does not, pass the setting synthParametersFromSettings reads (debugApp) or set CDK_DEBUG/JSII_HOST_STACK_TRACES through a path toolkit-lib honors, and restore a test asserting the env the app process receives.
| export async function runSynth(options: SynthRunnerOptions): Promise<SynthRunResult> { | ||
| const app = readCdkConfig(options.projectDir).app; | ||
| if (app === undefined) return { status: 'unavailable' }; | ||
|
|
||
| let cached; | ||
| let asm; | ||
| try { | ||
| const cx = await options.toolkit.fromCdkApp(app, { | ||
| workingDirectory: options.projectDir, | ||
| const cx = await options.toolkit.fromCdkAppDirectory(options.projectDir, { | ||
| lookups: false, | ||
| // Make jsii forward host-language (Python/Java) source frames; TS resolves via source maps. | ||
| env: { JSII_HOST_STACK_TRACES: '1' }, | ||
| synthOptions: { | ||
| debug: true, | ||
| }, |
There was a problem hiding this comment.
🟡 Recommended — This hunk drops the explicit JSII_HOST_STACK_TRACES: '1' env var (commented "Make jsii forward host-language (Python/Java) source frames"), and the companion host-language branch in source-resolver.ts is removed too. Even if TypeScript source mapping is restored, JSII_HOST_STACK_TRACES is only set by toolkit-lib's synthParametersFromSettings under debugApp, so a Python/Java user who previously got go-to-source silently loses it. The PR is titled as a feature-preserving refactor, yet this is an observable behavior change with no stated justification.
Suggested change: Confirm whether host-language support is intentionally dropped. If so, state it in the PR body; if not, keep forwarding host frames so Python/Java source anchoring continues to work.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| export async function readAssemblyDirectory(toolkit: Toolkit, abort: AbortSignal, assemblyDir: string) { | ||
| const asmSource = await toolkit.fromAssemblyDirectory(assemblyDir); | ||
| // NOTE: Purposely using asmSource.produce() here: toolkit.synth() would throw on validation failures and we don't necessarily want that. |
There was a problem hiding this comment.
🟡 Recommended — readAssemblyDirectory calls toolkit.fromAssemblyDirectory(assemblyDir) with no options, but the code this replaces passed { failOnMissingContext: false }. In toolkit-lib AssemblyDirectoryProps.failOnMissingContext defaults to true, so produce() now throws when the assembly's manifest.missing is non-empty — a common state for any cdk.out synthesized with unresolved context lookups. The server catches that rejection into its error branch, which only logs and returns with no cached index, so the Explorer silently shows no constructs, lenses, or diagnostics for that project. The PR body does not mention this behavior change.
Suggested change: Pass { failOnMissingContext: false } to toolkit.fromAssemblyDirectory(...) in readAssemblyDirectory, restoring the prior intent — or, if failing on missing context is deliberate for the Explorer, say so in the PR body.
| @@ -23,17 +15,10 @@ export interface SourceLocation { | |||
| */ | |||
| export class SourceMapResolver { | |||
There was a problem hiding this comment.
⚪ Optional — The comment reads // Parsed source maps keyed by absolute .js path; null = known to have no map., but the cache is written by loadTraceMap(jsFile: RelativePath) and read by reverseMapJs via this.cache.get(loc.file) — both using the assembly-relative path, not an absolute one — and the "no map" sentinel is undefined, not null. The comment misleads the next reader about both the key space and the sentinel value.
Suggested change: Update the comment to: "Parsed source maps keyed by assembly-relative .js path; undefined = known to have no map."
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| describe('runSynth', () => { | ||
| test('reads the app from cdk.json, returns success, and disposes the cached assembly', async () => { | ||
| test('reads the app from cdk.json, sets the right environment variables, returns success, and disposes the cached assembly', async () => { | ||
| const { toolkit, cached } = makeToolkit({}); | ||
| const projectDir = makeProjectDir({ app: APP }); | ||
|
|
||
| const result = await run(toolkit, projectDir); | ||
| // WHEN | ||
| await run(toolkit, projectDir); | ||
|
|
||
| expect(result).toEqual({ status: 'success' }); | ||
| expect(toolkit.fromCdkApp).toHaveBeenCalledWith(APP, { workingDirectory: projectDir, lookups: false, env: { JSII_HOST_STACK_TRACES: '1' } }); | ||
| // THEN |
There was a problem hiding this comment.
⚪ Optional — The first test is titled "reads the app from cdk.json, sets the right environment variables, returns success, and disposes the cached assembly", but after the rewrite it no longer asserts any environment variable — the old JSII_HOST_STACK_TRACES assertion was removed and nothing replaced it, and the fake fromCdkAppDirectory does not inspect synthOptions. The title now describes a check the test does not perform, hiding the untested env-var regression behind an inaccurate name.
Suggested change: Either assert the env/synthOptions the runner now passes, or drop the "sets the right environment variables" clause from the title so it matches what the test checks.
| } | ||
| } | ||
|
|
||
| // One synth, retrying the write lock with exponential backoff + full jitter. | ||
| // The latch guarantees we never contend with our own synth, so a lock-conflict | ||
| // here is an external writer (a terminal `cdk synth`/`cdk watch`) holding | ||
| // cdk.out; retry until it releases rather than dropping the synth. | ||
| async function runSynthWithRetry(): Promise<SynthRunResult> { | ||
| let delayMs = SYNTH_LOCK_RETRY_BASE_MS; | ||
| const deadline = Date.now() + 3_600_000; // 1 hour | ||
| while (Date.now() < deadline) { | ||
| const result = await (synthRunner ? synthRunner(currentProjectDir()) : Promise.resolve({ status: 'error', message: 'No synth runner configured' } as const)); | ||
| if (result.status !== 'lock-conflict') { | ||
| publishSynthDiagnostics(result); | ||
| return result; | ||
| } | ||
| await sleep(Math.random() * delayMs); | ||
| delayMs = Math.min(delayMs * 2, SYNTH_LOCK_RETRY_MAX_MS); | ||
| } | ||
| const timeoutResult: SynthRunResult = { status: 'error', message: 'Auto-synth timed out waiting for external writer to release cdk.out' }; | ||
| return timeoutResult; | ||
| } | ||
| // Concurrency latch, mirroring `cdk watch` (toolkit.ts): a synth requested | ||
| // while one runs is batched into a single follow-up, so synths run one at a | ||
| // time and the assembly catches up to the latest save. | ||
| let synthLatch: SynthLatch = 'open'; | ||
| const synthListeners: Array<() => void> = []; | ||
|
|
||
| /** | ||
| * Queue a new synth | ||
| * | ||
| * Start it immediately if we are not currently synthing; otherwise, another instance | ||
| * of this async function is already running. Mark the latch as | ||
| * queued and the existing instance will check it in a loop. | ||
| * | ||
| * Returns a Promise that resolves once the queued synth has completed. | ||
| */ | ||
| function queueSynth() { | ||
| const ret = new Promise<void>((resolve) => { | ||
| synthListeners.push(resolve); | ||
| }); | ||
|
|
||
| // Shared by the manual CodeLens command and auto-synth-on-save. A call while a | ||
| // synth runs is batched (latch 'queued') and returns lock-conflict now, so the | ||
| // caller still sees "in progress" while the assembly catches up to the latest | ||
| // save. Any number of overlapping calls collapse into that one follow-up. | ||
| async function guardedSynth(): Promise<SynthRunResult> { | ||
| if (synthLatch !== 'open') { | ||
| if (synthLatch === 'open') { | ||
| void runSynthLoop(); | ||
| } else { | ||
| synthLatch = 'queued'; | ||
| return { status: 'lock-conflict' }; | ||
| } | ||
| synthLatch = 'synthing'; | ||
| try { | ||
| // Drain the queued slot with a loop (not recursion) so the stack stays | ||
| // flat. There is no await between reading the latch as not-'queued' and the | ||
| // finally resetting it to 'open', so under the single-threaded event loop | ||
| // any request that set 'queued' did so during an await and is observed | ||
| // here -- no wakeup is lost. | ||
| let result = await runSynthWithRetry(); | ||
| while ((synthLatch as SynthLatch) === 'queued') { | ||
| synthLatch = 'synthing'; | ||
| result = await runSynthWithRetry(); | ||
|
|
||
| return ret; | ||
|
|
||
| async function runSynthLoop() { | ||
| try { | ||
| do { | ||
| synthLatch = 'synthing' as SynthLatch; | ||
|
|
||
| // Get the current listeners and notify them once we are done, successfully | ||
| // or otherwise. | ||
| const currentListeners = synthListeners.splice(0, synthListeners.length); | ||
| try { |
There was a problem hiding this comment.
🟡 Recommended — When a synth is queued during a running synth, its resolver is pushed onto synthListeners and the loop drains and resolves listeners each iteration. But the loop's continuation guard is while (synthLatch === 'queued' && !shutdown.signal.aborted). If shutdown.abort() fires while a synth is queued but before the loop takes its next iteration, the loop exits without resolving the already-queued listener, so that queueSynth() promise never resolves. The manual COMMAND_SYNTH_NOW path awaits this promise through notify.withProgress(...), so on shutdown-during-queued-synth the progress indicator can hang unresolved.
Suggested change: In the finally that sets synthLatch = 'open', also drain and resolve any remaining synthListeners so queued callers are released on abort.
Move the explorer's core layer onto `AssemblyIndex` and reshape it so the LSP (and a future web UI) share one read model of a synthesized app. - `AssemblyIndexWithSourceLocations` wraps the index with source-location resolution, source-map reverse mapping and a line-to-constructs lookup per source file - paths are branded (`RelativePath` + `PathTranslator`), so app-relative, assembly-relative and absolute paths can no longer be mixed up, and `safe-url.ts` is the only way to turn one into a URL (enforced by a `no-restricted-imports` rule) - `source-watcher.ts` watches authored sources; `from-cdk-app-directory.ts` centralizes opening an app directory - `template-ranges.ts` is copied in from `cloud-assembly-api`, which has no other caller for it - `assembly-lock.ts` is gone: toolkit-lib now exports `SYNTH_LOCK_FILE`, which is all the watcher needed - LSP features (hover, codelens, diagnostics, template locator, server) are rewritten against the new core, with construct summaries split out into `summarize.ts` and logging into `log-sink.ts` - test fixtures replaced by `_helpers/assembly.ts` + `_helpers/fake-toolkit.ts` This leaves `construct-tree.ts` and `template-ranges.ts` in `cloud-assembly-api` without callers; a follow-up removes them.
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Total lines changed 3136 is greater than 1000. Please consider breaking this PR down. |
Move the LSP Explorer's loading layer onto the newly added
AssemblyIndex.Source locations and path handling
This adds an explorer-specific
AssemblyIndexWithSourceLocationson top which manages source mapping on top ofAssemblyIndex. It keeps track of the definition locations of constructs in source files and lines.There is a security concern that the Explorer should only be allowed to point to file paths below the application and assembly directories. In order to avoid confusion between absolute paths and relative paths, this change introduces type brands for
RelativePathandAbsolutePath, and introduces functions to map between them and to (absolute) URLs.Other changes
fromCdkAppDirectory(), a Cloud Assembly Source builder that readscdk.jsonand configures itself based on that.cloud-assembly-apiintocdk-explorer.This leaves
construct-tree.tsandtemplate-ranges.tsincloud-assembly-apiwithout callers; a follow-up removes them.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license