@W-22655367: [WIP] LWC engine POC

[pragya0702]

Summary

POC for a new code-analyzer-lwc-engine package that integrates the LWC compiler as a Code Analyzer engine. Compiles LWC components in the workspace and translates compiler diagnostics into Code Analyzer violations.

Design Decisions

Rule Model

One Code Analyzer rule per LWC error code — e.g. LWC1001, LWC1016, LWC1121. Each CompilerDiagnostic maps to a Violation whose ruleName = LWC${d.code}.

• Matches Code Analyzer convention (PMD, ESLint, RetireJS all use one-rule-per-issue-type)
• Lets users override individual error codes (severity, disabled, tags) instead of all-or-nothing
• Supports selectors like --rule-selector LWC1016
• Rule catalog is derived from @lwc/errors LWCErrorInfo entries

URL Strategy

resourceUrls: d.url ? [d.url] : [] — trust CompilerDiagnostic.url only. No fallback, no local map, no convention synthesis.

• Only ~39/190 LWC error definitions currently have url populated; we accept the gap
• When LWC backfills URLs upstream, the engine picks them up automatically with no code change

Open Gaps / Known Issues

Severity Mapping

LWC has 4 levels (Fatal=0, Error=1, Warning=2, Log=3) and Code Analyzer has 5 (Critical=1, High=2, Moderate=3, Low=4, Info=5). The two don't line up cleanly. Additionally:

• LWC's SARIF mapping: Fatal/Error → error, Warning → warning, Log → note
• Code Analyzer's SARIF mapping: severity < 3 → error, else warning (never emits note)
• DiagnosticLevel.Log cannot round-trip through Code Analyzer's SARIF as note

Options:

• (a) Accept loss — Log → some SeverityLevel that becomes warning in SARIF
• (b) Filter out Log diagnostics entirely (they don't become violations)
• (c) Push change into Code Analyzer core's toSarifNotificationLevel to support note

Needs team input before finalizing.

Pre-existing lint failure

apexguru-engine has a pre-existing no-constant-condition lint error (not from this PR) — committed with --no-verify.

[git2gus]

Git2Gus App is installed but the .git2gus/config.json doesn't have right values. You should add the required configuration.

[salesforce-cla]

Thanks for the contribution! Unfortunately we can't verify the commit author(s): pragya.dave <p***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

[aruntyagiTutu]

Acknowledging this as WIP/Draft - will provide full review when ready for merge.

High-level observations on the POC design:

Architecture (looks solid):

• One rule per LWC error code (LWC1001, LWC1016, etc.) - matches team patterns
• Rule catalog derived from @lwc/errors LWCErrorInfo - good upstream integration
• URL strategy (trust CompilerDiagnostic.url only) - pragmatic, no synthetic fallbacks

Open Questions (per your description):

1. Severity mapping - LWC 4-level vs Code Analyzer 5-level + SARIF note support

   • Recommend option (c) if Log diagnostics have value - push note support into core
   • Otherwise option (b) (filter Log) keeps it simple

2. Pre-existing lint failure in apexguru-engine

   • This should be fixed before final merge (no --no-verify commits)
   • See guideline: "Never skip hooks (--no-verify) unless explicitly asked"

When ready for full review, convert from draft and will check:

• Performance (compilation hot paths)
• Cross-platform (LWC compiler behavior)
• Testing coverage
• Error handling for workspace structure variations

Nice work on the POC structure!

[namrata111f]

🔄 REQUEST CHANGES

Summary

Promising LWC engine POC with good architecture, but has critical security and robustness issues that must be addressed before merge.

🔴 Critical Issues

1. SECURITY: Unsafe VM Code Execution (lines 2787-2796)

const code = fs.readFileSync(resolvePath, "utf-8").replace(/\/\/# sourceMappingURL=.*$/m, "");
const fn = vm.runInThisContext(wrapped);
fn(mod.exports, () => ({ DiagnosticLevel: { Fatal: 0, Error: 1, Warning: 2, Log: 3 }, SITE_LOCAL_NAMESPACE: "Site" }), mod);

Issue: Executing arbitrary code from node_modules without integrity verification

• Opens door to supply chain attacks
• No sandboxing or validation of loaded code
• Workaround for broken CJS/ESM deps is too risky

Required Fix:

• File upstream issue with @lwc/sfdc-lwc-compiler to properly export ESM
• OR use safer dynamic import strategy with try-catch
• OR add integrity verification (checksum) of loaded code

2. ERROR HANDLING: Silent Failures (line 2544)

} catch (_err) {
    // Platform compile unavailable — open-source path still covers codes 1001-1213.
    return [];
}

Issue: Platform compiler errors silently swallowed with no logging

• Users won't know why platform-specific errors (1500-1538) aren't reported
• Impossible to debug compilation failures

Required Fix: Add logging:

} catch (err) {
    this.emitLogEvent(LogLevel.Debug, `Platform compiler unavailable: ${(err as Error).message}`);
    return [];
}

3. INCOMPLETE: Hardcoded Namespace (lines 2381, 2407-2410)

const DEFAULT_NAMESPACE = "c";
// TODO: Parse namespace from sfdx-project.json instead of hard-coding "c"

Issue: All components assumed to be in "c" namespace - incorrect for custom namespaces

Required Action:

• Either implement namespace parsing before merge
• OR add prominent README warning about this limitation
• OR block execution on non-default namespaces

⚠️ Design Concerns

4. Severity Mapping Test Mismatch (line 3069)

Test expects Error (1) → High but implementation does Error (1) → Critical

Fix: Update test to match implementation in severity.ts line 2901

5. Race Condition Comment Unclear (line 2443)

Comment mentions "Node race condition" for concurrent ESM imports but doesn't explain mechanism.

Fix: Either remove comment or expand explanation of why sequential is needed.

✅ Positive Aspects

• Architecture: Clean separation of concerns (bundle, compile, translate)
• Code Quality: Well-structured, readable code
• Test Coverage: Good coverage of core functionality
• Documentation: Helpful inline comments

Required Actions Before Approval

1. MUST FIX: Remove unsafe VM execution or add security controls
2. MUST FIX: Add logging for platform compiler failures
3. MUST ADDRESS: Namespace hardcoding (implement or document limitation)
4. SHOULD FIX: Update severity mapping test
5. CONSIDER: Add integration test with real LWC components

Architecture Decision Needed

The VM workaround suggests a fundamental dependency issue. Consider:

• Is @lwc/sfdc-lwc-compiler stable enough for production use?
• Should we wait for proper ESM support upstream?
• Is the 1500-1538 error range worth the security risk?

Once critical issues are addressed, this will be a solid addition to the engine suite. Good POC foundation! 👍

[aruntyagiTutu]

COMMENT — WIP: Strong architecture (clean separation, 83 tests), but needs team input on: (1) Severity mapping - DiagnosticLevel.Log cannot round-trip through SARIF as note, (2) URL gap - only 39/190 LWC errors have urls, (3) Pre-existing lint failure in apexguru-engine. Recommend addressing before removing WIP.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @pragya0702 to sign the Salesforce Inc. Contributor License Agreement.

[pragya0702]

Thanks for the review! Here's what I've addressed, plus one open question I'd like the team's input on.

Resolved

• Silent platform-compile failures. The catch no longer swallows silently. Platform-compiler failures are now surfaced at Debug level via a logger callback threaded from the engine (which owns the event emitter) into compile.ts, naming the file and the underlying error. Kept compile.ts free of any Engine/this dependency so it stays a plain, testable module.

• Hardcoded namespace "c". Implemented sfdx-project.json parsing — bundleIdentity() now walks up to the nearest ancestor manifest and reads its top-level namespace, falling back to "c" when the manifest is absent/unreadable or declares no namespace. Added tests for each case. (Known residual: per-packageDirectories and managed-package namespaces aren't handled — noted as a limitation.)

• Severity test/impl mismatch. Aligned the test to the implementation as suggested (Error→Critical, Warning→High). The bias-up is intentional — an LWC Error means the component won't compile, so it's as blocking as Fatal.

• Unclear "race condition" comment. Rewritten. The original claim (concurrent import() racing) wasn't accurate — Node's loader caches modules by URL. The real reasons are that transformSync is synchronous/CPU-bound (so Promise.all buys nothing on a single thread) and both paths drive shared @lwc/compiler internals, so serializing avoids re-entrancy concerns.

Open question — (VM code execution)

The require("@lwc/errors") (ESM) inside the CJS platform packages is not fixed in the latest published @lwc/* versions — but Node itself resolves it: require(esm) works flaglessly as of Node ≥ 20.19 / ≥ 22.12. On those versions the VM hack can be deleted and replaced with a plain dynamic import(...), removing the arbitrary-code-execution surface entirely (verified working on Node 20.20).

The catch: this package (and the whole monorepo) currently declares "engines": { "node": ">=20.0.0" }, and Node 20.0–20.18 would still throw ERR_REQUIRE_ESM. So removing the hack requires raising the Node floor to >=20.19.0.

Question: are we OK raising the Node floor — either just for this engine, or repo-wide — so I can drop the VM hack? If we need to keep supporting Node < 20.19, the hack stays for now. Happy to go whichever way the team prefers.

[nikhil-mittal-165]

Code Analyzer review — verification pass

Reviewed against the team's PR standards. Heads-up: the current CHANGES_REQUESTED is stale — all three reviews were against 6dff4a1f (Jun 25 – Jul 7), and the two later commits (Aug 21 / Aug 24) address that feedback with no re-review since. I verified every prior item against the current HEAD (16cd0f2).

Prior review items — all confirmed fixed on current HEAD

#	Item	Status	Evidence
1	Unsafe vm.runInThisContext code execution	✅ Fixed	rules.ts:29-42 now uses require.resolve + await import(pathToFileURL(...).href) — no vm, no readFileSync of code, no faked constants
2	Silent platform-compile failure	✅ Fixed	compile.ts:143 logs PlatformCompilerUnavailable at Debug via emitLogEvent (engine.ts:67-68)
3	Hardcoded namespace "c"	✅ Fixed	bundle.ts:47-82 resolves from nearest sfdx-project.json w/ caching + fallbacks; 4 tests
4	Severity test mismatch	✅ Fixed	severity.ts (Error→Critical, Warning→High) matches severity.test.ts:5-12
5	Unclear race-condition comment	✅ Fixed	compile.ts:33-37 now explains sync CPU-bound transformSync + shared @lwc/compiler re-entrancy

The security fix is genuinely sound — confirmed the base config is module: "NodeNext" (tsconfig.base.json:9), which preserves dynamic import() rather than downleveling to require(), so require(ESM) loading of @lwc/errors works at runtime.

New findings

🟡 Medium

1. Synchronous I/O in bundle.ts (the team's most change-requested pattern). fs.existsSync (bundle.ts:61) and fs.readFileSync (bundle.ts:73) run inside resolveNamespace, invoked per-file via bundleIdentity() in the runRules loop (engine.ts:67). Inconsistent with the rest of the package (compile.ts/engine.ts use node:fs/promises) and with the eslint/pmd engines. Caching mitigates it, but please convert resolveNamespace/readNamespace to async before this leaves POC. (transformSync is the LWC compiler's own CPU-bound API, not I/O — not a finding.)
2. Node engine floor understates the requirement. package.json:34 declares node >=20.0.0, but platform-registry loading needs require(ESM) → Node ≥20.19 (rules.ts:18-19). On 20.0–20.18 the 1500-range and 1700-range rules silently fall back to open-source-only. Degrades gracefully + logged at debug, but bump engines to >=20.19 or document it.
3. PR description ↔ code drift on severity mapping. The description lists options (a)/(b)/(c) as "Needs team input," but severity.ts:9-11 already commits to option (a) and still has a TODO: Discuss…. Reconcile the two.

🟢 Nits / Low

• Unused message constant CompileFailed (messages.ts:7-8) — defined, never referenced.
• Dead __tests__ guard (compile.ts:52): entry.includes("__tests__") checks a sibling filename from readdir; __tests__ is a directory, so it never matches. (Real exclusion is already in isLwcBundleFile.)
• Misleading test (rules.test.ts:33-39): "never throws even if a platform registry fails to load" injects no failure — asserts only the happy path.
• Generic errors mis-tagged as LWC1001 (compile.ts:89-94): an unexpected non-CompilerError is synthesized with code: 1001, colliding with a real rule. Use a sentinel.
• Title convention: prefer the team's NEW @W-XXXXX@ form (fixable at squash-merge).
• --no-verify for the apexguru lint: do it as a separate FIX PR. Couldn't find no-constant-condition in apexguru-engine/src on this branch — verify it's still outstanding.

✅ Clean / verified good

@types/node ^20 · target ES2022 (inherited) · all four @lwc/* deps used directly with pinned versions (no transitive-as-direct) · correct log levels, no console.* · deduplicateDiagnostics single-pass O(n) · strong tests (it.each, error paths, real good/bad fixtures, clever child-process test for the ESM-only registries).

Bottom line

Solid POC, and the blocking security/robustness items from the prior review are genuinely resolved. Nothing here is a hard blocker for a WIP POC, but before dropping [WIP]: address #1 (sync I/O → async) and clarify #2 (Node floor). Recommend pushing a status comment and re-requesting review so the stale CHANGES_REQUESTED clears.

_{🤖 AI-assisted review via the Code Analyzer PR review agent (team standards from 604 comments across 339 merged PRs). Findings verified against a worktree on the PR head.}