NEW @W-23659201@ Add uibundle engine to Code Analyzer Core

[amritmishra-sf]

Summary

Adds a new SFCA v5 engine plugin @salesforce/code-analyzer-uibundle-engine that validates UI Bundle build output.

The engine is named generically (uibundle) so additional UI-bundle rule families can be added later without a package rename. The initial ruleset ships 8 sourcemap-integrity rules.

What's included

• New package packages/code-analyzer-uibundle-engine/ — engine class UIBundleEngine (NAME = uibundle), plugin UIBundleEnginePlugin, following the sibling-engine layout.
• 8 rules: missing-sourcemap, path-leakage, invalid-source-references, vlq-integrity, source-content-verification (Critical), coverage-analysis, structural-coherence, token-consistency.
• Rule descriptions i18n via getMessageFromCatalog, goldfile-tested at test/test-data/uibundle-engine-goldfile.json.
• Whitelist entry in .node-scripts/validate-changed-package-versions.js for the not-yet-published package.

Test plan

• npm run build — clean
• npm run lint — clean
• npx jest --coverage — 69/69 pass, 4 suites
• Coverage: 91.17% stmt / 81.67% branch / 98.92% funcs / 93.85% lines — clears the 80% root gate.

Companion PR

CLI-side registration: forcedotcom/code-analyzer#2080 — pins @salesforce/code-analyzer-uibundle-engine@0.1.0-SNAPSHOT, so it can only go green once this engine is published. Sequence the merge accordingly.

Related

• Work item: W-23659201

[git2gus]

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

[aruntyagiTutu]

Reviewed the new code-analyzer-uibundle-engine package (engine.ts, plugin.ts, messages.ts, rules.ts, all 8 validators, and tests). Solid first cut — async I/O throughout (no sync fs calls), good use of caching (sourceAstCache, embeddedCache), tests have real assertions and cover error paths (missing sourcemap, bad VLQ, leaked paths, missing source refs), and the PR description accurately matches what's implemented (8 rules, goldfile test, companion PR sequencing noted).

A few non-blocking suggestions:

1. Redundant source-tree indexing across validators (perf). source-content-verification.ts, structural-coherence.ts, and token-consistency.ts each independently walk + read + index the entire source tree via their own indexSourceFiles(). When all three rules run together (the default), the source tree gets walked and every file re-read/re-indexed 3x. Per the team's "minimize passes over data" guideline, consider hoisting the source index into runOnTarget() in engine.ts and passing a shared index into each validator, so it's built once per target regardless of how many source-dependent rules are selected.

2. Duplicated helper code. indexSourceFiles, expandIndexWithBase, and INDEX_IGNORE_PREFIXES are copy-pasted verbatim between structural-coherence.ts and token-consistency.ts (and a near-identical variant lives in source-content-verification.ts). Worth extracting into sourcemap-io.ts as a shared utility — would also make suggestion #1 easier to implement in one place.

3. findNodeAtOffset's early break (source-content-verification.ts) assumes nodes[] is sorted by byteOffset. That holds today because collectSignificantNodes relies on Babel's enter-order traversal producing non-decreasing start offsets, but it's an implicit invariant, not asserted or documented at the call site. If that ever changes (e.g. a traversal tweak), the break would silently drop valid matches rather than erroring. A short comment noting the sortedness assumption (or an explicit sort before this loop) would make it safer to modify later.

4. Minor: DANGEROUS_API_PATTERNS builds "eval(" and "Function(" via ["ev","al","("].join("") style construction with no comment explaining why — presumably to avoid this scanner's own dangerous-pattern list from tripping other static-analysis tools on itself. A one-line comment would save the next reader some head-scratching.

None of these block merge — nice addition to the engine lineup.

[amritmishra-sf]

[nikhil-mittal-165]

is the license file present for all engines ?

[amritmishra-sf]

yes, it seems like other engines have this too

[nikhil-mittal-165]

Automated review — Code Analyzer team standards

Reviewed against the team's PR review standards (performance, naming, validation, logging, testing, compatibility, cross-platform, dependencies). Checked out the PR branch and read every changed source/test/doc file directly.

Overall verdict: Changes Requested

Per this team's own trigger list, two things independently qualify: a performance-sensitive hot path shipped with zero large-project measurement, and a correctness bug in the Critical-severity rule that causes false positives.

---

🔴 Blocking

Performance — source-content-verification.ts (Critical rule, 520 lines)

• findNodeAtOffset linear-scans from index 0 on every call even though the node array is sorted by byteOffset — O(N×M) per compiled file. Should be a binary search.
• indexSourceFiles loads the entire source tree into memory with no bound — unsafe for "thousands of files" projects.
• No test/measurement on a large project despite this being exactly the scenario the team requires evidence for.

Correctness — source-content-verification.ts:218

• The "byte-equal" check does embedded.trim() !== submitted.trim(), which only trims the ends. It doesn't normalize internal CRLF vs LF, so a CRLF checkout vs LF-embedded sourcesContent triggers false SourceContentBytewiseMismatch findings — on the Critical rule. No CRLF/BOM test exists.

Performance — structural-coherence.ts / token-consistency.ts

• Both re-split the entire indexed source tree (structural-coherence, per dist file) or the entire compiled bundle (token-consistency, per sampled mapping) instead of precomputing once. Same O(N·M) class of bug.

Correctness — Windows path separators (duplicated bug in both files)

• indexSourceFiles keys its map with path.relative(...) (backslashes on Windows) while lookups use forward-slash-normalized keys → the submitted-source index silently never matches on Windows, degrading both rules to embedded-content-only. Untested.

Math bug — structural-coherence.ts:87

• Whitespace ratio denominator (totalMappingsChecked / 10) doesn't match the actual sample count taken (% 10 === 0), so the ratio can exceed 100%.

Test suite — vacuous assertions

• At least 4 tests in validators-integration.test.ts (whitespace-heavy, cross-file-jump, AST-compat, sourcemap-unloadable) assert only length >= 0 or Array.isArray(...) === true — tautologies that can never fail. Notably, these are exactly the tests that should have caught the whitespace-ratio bug above and didn't.

Architecture — DRY violation

• indexSourceFiles/expandIndexWithBase/dist-walk boilerplate is duplicated byte-for-byte between structural-coherence.ts and token-consistency.ts instead of living in the shared sourcemap-io.ts. This is also why the Windows bug had to be fixed in two places.

🟡 Medium (worth resolving before merge)

• path-leakage.ts: absolute-path detection only catches /Users|home|root, drive letters, UNC, file:// — misses common CI/Docker paths like /app, /build, /opt, contradicting the rule's own stated purpose.
• Sourcemap re-read/re-parsed independently by up to 3 validators (path-leakage, invalid-source-references, vlq-integrity) instead of once and shared.
• sourcemap-io.ts silently drops malformed-JSON maps assuming vlq-integrity will report it — breaks if that rule is deselected/run in isolation.
• missing-sourcemap.ts: double-scans and double-reads the same file content (classification pre-check + local re-filter; orphan-file read done twice).
• coverage-analysis.ts: only inspects the first mapped column per line, so a single-line minified bundle (the common real-world case) can read ~100% coverage regardless of actual gaps.
• engine.ts: 3 message-catalog entries (NoBundleTargetsFound, SkippedForTarget, SkippedNoSourceTree) are dead code — the same strings are hardcoded inline instead, risking drift.
• engine.ts: per-target Warn logs aren't aggregated (violates the "consolidate repetitive logs" standard).
• Missing error-path tests: corrupt VLQ data, non-string mappings, segment-index-out-of-range (vlq-integrity); engine dispatch skip branches; token-consistency malformed-map path — consistent with branch coverage sitting at 81.67% vs 91-99% elsewhere.
• startColumn: 1 in 3 validators vs the engine's documented 0-based convention → reports column 2 instead of 1; inconsistent with vlq-integrity.ts which omits it correctly.
• Whitelist entry in .node-scripts/validate-changed-package-versions.js has no "remove once published" marker, so it'll silently disable version-bump checking for this package forever.
• Test temp dirs (makeTmpDir) are never cleaned up (afterEach/afterAll missing) — accumulates in CI over time.

🟢 Low / nits

• Ambiguous idx naming in engine.ts:202 (really "last dist-segment index"); describeRules() returns the shared mutable RULES array by reference instead of a copy; .includes() used where the source is already Set-derived.
• Regex used where plain char comparisons would do (classifyTokenAt, extractWordAt, pointsToWhitespaceOrComment).
• Several it() blocks in engine.test.ts are it.each() candidates (near-identical rule/fixture variants).
• messages.ts uses "does not"/"do not" instead of contractions, inconsistent with the team's doc style (internally consistent, so low priority).
• Sourcemap JSON parsed twice (JSON.parse then new TraceMap re-parsing the same string).

✅ Clean

• No sync I/O anywhere in the runtime code — all fs.promises.
• @types/node correctly pinned at ^20.0.0; tsconfig/eslint config byte-identical to sibling engines; no unjustified version bumps.
• PR description accurately matches the code — all 8 rule names/severities verified against rules.ts/messages.ts.
• No unused/duplicated-transitive dependencies; @babel/* bumps in the lockfile are minor/patch only.
• Commit headlines follow NEW @W-XXXXX@ / DOC @W-XXXXX@ convention.
• Engine contract (describeRules/runRules) is tested end-to-end with real temp bundles, not over-mocked; 69/69 tests confirmed to actually exist as claimed.

---

Bottom line: solid first cut of a new engine with good structural conventions (sibling-package parity, message catalog pattern, async I/O throughout), but the three biggest sourcemap-analysis validators (source-content-verification, structural-coherence, token-consistency) all independently reinvented an expensive per-item re-scan instead of precomputing once, and two of the vacuous tests mean a real bug (the >100% whitespace ratio) already slipped through review-by-test. Recommend addressing the perf fixes + CRLF bug + Windows path bug before merging; everything else can be follow-up comments.

Generated via automated review against the Code Analyzer team's PR standards (604 review comments / 339 merged PRs analysis).

[aruntyagiTutu]

Thanks for the fix — mirroring the Layer-1 virtual-source skip (isVirtualSource/isDependency/isAsset) into the AST orphan check in runAstChecks makes sense, and the comment explaining why (bundler pseudo-sources like ?raw/webpack/vite internals aren't part of the submitted tree, with the ratio gate still catching abuse) is clear. Severity alignment (Critical→High for source-content-verification, matching the other Layer-1 rules) also seems reasonable.

One non-blocking gap: I don't see a new test that exercises this specific AST-path fix — e.g. a mapped node whose orig.source resolves to a virtual/dependency/asset path, asserting no "not present in the submitted source tree" (orphan) finding is raised. The existing virtual-source tests (does not flag virtual or relative sources, skips virtual and remote sources, `flags a virtual-source ratio above threshold") cover other validators/the Layer-1 byte-equal gate, but not this AST branch specifically. Since this was a real false-positive bug, a regression test would help guard against it recurring.

Not blocking — happy to approve once tests pass in CI.

[amritmishra-sf]

Removed UI Bundle from the default scans

[aruntyagiTutu]

Thanks for these fixes — all look correct:

• CRLF/CR normalization before byte-equal comparison and forward-slash normalization on path.relative output resolve real Windows correctness bugs (source indexing/lookup would otherwise fail on Windows checkouts).
• The whitespace-ratio fix in structural-coherence.ts is right — whitespaceSampleCount now tracks actual sample fires (matching the sampleIndex % INTERVAL === 0 condition) instead of floor(total/10), which could previously push the ratio over 100%.
• The strengthened test assertions (replacing Array.isArray(...)/length >= 0 with real checks for byte-mismatch, unloadable-sourcemap, whitespace, and cross-file-jump findings) are a solid improvement — these now actually verify behavior instead of trivially passing.
• Removing MIGRATION.md and fixing the tsc tuple-typing issues in test helpers are sensible cleanup.

LGTM.

[nikhil-mittal-165]

remember to revert this piece of code post PR merge

[amritmishra-sf]

I accidentally, removed this now. It seems to have broken the build. I will add it back and revert it post merge

[aruntyagiTutu]

Commit 299ceccc removes packages/code-analyzer-uibundle-engine from the unpublished-package whitelist in .node-scripts/validate-changed-package-versions.js, but the package still isn't published to npm (confirmed: npm view @salesforce/code-analyzer-uibundle-engine version → 404).

This will crash the verify-pr.yml version-check step for this PR itself (and any future PR touching this package) — not just fail a check, but throw an uncaught exception:

• getLatestReleasedVersion catches the npm view 404 and returns undefined (handled fine).
• But then semver.parse(undefined) returns null (not a throw), and the subsequent semver.lte(semver.parse(currentVersion), null) throws Invalid version. Must be a string. Got type "object" — uncaught, crashing the script with a nonzero exit.

I verified this locally against the repo's actual semver dependency:

semver.parse(undefined) // => null
semver.lte(semver.parse('0.1.0'), null) // throws "Invalid version. Must be a string. Got type \"object\""

Since this same commit still touches files under packages/code-analyzer-uibundle-engine, the next CI run for this PR should hit this path. Recommend keeping the whitelist entry until the package is actually published (revert this piece of 299ceccc), or hardening identifyIncorrectlyVersionedPackages to treat an unresolvable releasedPackageVersion as "not yet published → skip" rather than assuming npm view failing implies a comparable prior version exists.

The rest of this push (comment trimming across the uibundle validators, dropping the // Babel columns are 0-based / // SFCA requires... type comments) is a reasonable readability cleanup with no functional change — no concerns there.

[aruntyagiTutu]

Confirmed — the whitelist entry for packages/code-analyzer-uibundle-engine is restored in 7b6b7f9e, resolving the version-check crash from my previous review. The uibundle README removal (e91cbdc7) is also reasonable since messages.ts already documents each rule's behavior and a separate README would drift out of sync.

The remaining files in this diff (apexguru-engine changes) came in via the merge from dev (PR #500), not new work on this branch — no action needed there.

LGTM.

[ankitsinghkuntal09]

Verified sanity plus regression:
W-23659201-Testing-Scenarios: https://docs.google.com/document/d/1amoe4NyOCJZVK3RAjZy23jJX9W1TLVhJ/edit

[nikhil-mittal-165]

Updated review — tracking what's still open (posting for the record; please read past the green state)

Re-reviewed the current branch head against the team's PR standards after the fix commits. The correctness + CI-integrity blockers from the earlier round are genuinely resolved — credit where due. This comment intentionally tracks only what's still open so it doesn't get buried under the approvals.

✅ Confirmed fixed (no action needed)

• CRLF/CR normalization before the byte-equal check (normalizeLineEndings) — source-content-verification.ts:216
• Windows path separators normalized (toPosixPath(path.relative(...))) in all three source indexers
• Whitespace-ratio denominator now tracks actual sample fires — structural-coherence.ts:85
• The four vacuous integration tests (length >= 0 / Array.isArray) replaced with real behavioral assertions
• Whitelist entry restored — the verify-pr.yml version-check crash is resolved
• MIGRATION.md / README.md removed, comment bloat trimmed, severity Critical → High

---

🔴 Still open — please address or explicitly defer with a GUS follow-up

1. Missing regression test for the AST virtual-source skip (commit 35cd871).
This was flagged on 08-18 and never added. The false-positive fix in runAstChecks (source-content-verification.ts:277) shipped with no test — nothing asserts that a mapped node whose orig.source resolves to a virtual/dependency/asset path raises no orphan finding. It was a real false-positive bug; it needs a guard so it can't silently regress.

2. Performance / DRY cluster — never addressed (my earlier review + the two non-blocking suggestions on 08-17).
Mitigated only by removing the engine from default scans, not fixed in code:

• source-content-verification, structural-coherence, and token-consistency each independently walk + read + index the entire source tree — indexSourceFiles (plus expandIndexWithBase / INDEX_IGNORE_PREFIXES) is copy-pasted verbatim across all three. When all three run (the default selection), the tree is read and re-indexed 3×. Hoist a single shared index into runOnTarget() in engine.ts and pass it into each validator.
• analyzeCoherence re-splits every source into line-lengths per dist file, and classifyTokenAt / nameExistsNear call split("\n") on the full source on every sampled mapping. Precompute once per file.
• findNodeAtOffset (source-content-verification.ts:421) scans from index 0 on every call — O(N·M) on large files — and its early break silently assumes nodes[] is byte-offset-sorted (an undocumented invariant). Binary search + a comment, or at minimum document the invariant.
• No large-project measurement was done. Per our own standard, this path needs before/after numbers on a project with thousands of files before it's ever enabled by default.

3. coverage-analysis under-reports on the realistic case.
It credits everything from the first mapped column to end-of-line as "covered" (coverage-analysis.ts:129-133), so a single-line minified bundle — the common shipped shape — reads ~100% coverage regardless of internal gaps.

---

🟡 Medium

4. Column convention is inconsistent and off-by-one in places.
toViolation treats validator columns as 0-based and adds 1 (engine.ts:180). But:

• coverage-analysis.ts:63 emits startCol + 1 (already 1-based) → double-incremented, reports the column one too high
• path-leakage / missing-sourcemap / invalid-source-references emit hardcoded startColumn: 1 → reported as column 2
• only source-content-verification (raw 0-based) round-trips correctly

Pick one convention (validators emit 0-based, engine converts) and make all validators follow it.

5. path-leakage misses common CI/container roots.
isLeaking (path-leakage.ts:7-9) only catches /Users|home|root, drive letters, UNC, and file://. It misses /app, /build, /opt, /tmp, /var — exactly the absolute paths CI and Docker builds tend to leak, which is the rule's stated purpose.

6. Dead message-catalog entries + drift risk.
NoBundleTargetsFound, SkippedForTarget, and SkippedNoSourceTree exist in messages.ts but the engine hardcodes the same strings inline (engine.ts:71,116,137). Wire them up or delete them so the two copies can't drift.

7. collectSourceMaps silently drops malformed JSON (sourcemap-io.ts:29, "vlq-integrity surfaces malformed JSON") — breaks if vlq-integrity is deselected while path-leakage / invalid-source-references still run.

8. Test temp dirs are never cleaned up.
makeTmpDir is called ~40× with no afterEach/afterAll; the docstring itself concedes cleanup is left to the caller. These accumulate in CI over time.

9. Missing error-path tests — VlqDecodingFailed (corrupt VLQ) and the token-consistency malformed-map path are untested. Branch coverage sits at 81.7% vs 91–99% for statements, and this is where the gap is.

10. Whitelist has no in-code "remove once published" marker.
The revert is tracked only in a PR comment thread. Add a // TODO: remove once @salesforce/code-analyzer-uibundle-engine is published (W-23659201) next to the entry in validate-changed-package-versions.js so it isn't silently left disabling version-bump checks forever.

---

🟢 Nits

• Per-target Warn logs aren't aggregated (engine.ts) — consolidate per the logging standard.
• describeRules returns the shared mutable RULES array by reference (engine.ts:60) — return a copy.
• Ambiguous idx in engine.ts:200 (it's the last dist segment index).
• messages.ts uses "does not" / "do not" — team style prefers contractions.
• engine.test.ts rule cases are it.each() candidates.

---

Net: the merge-blockers are cleared, but items 1–3 are the ones I'd want resolved — or consciously deferred with a GUS follow-up — before this engine is ever switched into a default scan. Happy to pair on the shared-index refactor, since it collapses most of item 2 into one change.