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.

[aruntyagiTutu]

Reviewed the new commit (f2aac4c) addressing the outstanding performance review blockers on this PR. Verified each fix directly:

Shared source-index build (engine.ts + sourcemap-io.ts). buildSourceIndex() is now extracted into sourcemap-io.ts and built once per target in analyzeSourceRules, then threaded through as an optional sourceIndex param to validateSourceContent, validateStructuralCoherence, and validateTokenConsistency (each falls back to building its own via options.sourceIndex ?? await buildSourceIndex(...), so existing unit tests that call the validators directly without a shared index still pass unmodified). This eliminates the 3x redundant walk()-based directory scans per rule selection that were flagged before — a real fix, not just a comment tweak.

Binary search in findNodeAtOffset. Replaced the linear scan with a binary search for the lower bound (byteOffset - tol), then a bounded scan of the tolerance window. The early-break-on-sorted-order assumption already existed in the prior linear implementation (if (n.byteOffset > byteOffset + tol) break), so this isn't a new correctness assumption — it's a legitimate algorithmic speedup over the same precondition (Babel's pre-order traversal producing non-decreasing byte offsets for the filtered significant-node list).

Coverage over-credit fix in coverage-analysis.ts. This is the most substantive fix: previously a single mapping at column 0 credited the entire line as mapped regardless of length, which meant a 5000-char minified line with one mapping read as ~100% covered. The new logic caps each mapping's reach at UNMAPPED_THRESHOLD (50 chars) and records the remainder as an unmapped gap. Traced the math by hand against the new test case (single mapping at col 0, 5000-char line): mappedChars becomes 50, coveragePct ~1%, correctly triggering the "excessive unmapped" signal that was previously silently missed. This is exactly the kind of correctness bug this validator exists to catch, so good catch and fix.

Tests. New regression tests cover both the coverage over-credit scenario and the AST orphan-source false-positive case for virtual/dependency/asset sources — both assert on concrete outcomes rather than shape, consistent with the team's testing standards.

Also confirmed the .node-scripts/validate-changed-package-versions.js whitelist entry for packages/code-analyzer-uibundle-engine (fixed in an earlier commit) is still intact, so CI's version-check crash risk remains resolved. No new issues found.

[nikhil-mittal-165]

Automated re-review — post-f2aac4c (Code Analyzer team standards)

Re-reviewed after the "Address PR #499 review blockers" push (f2aac4c, 08-19). Method: fetched the PR head into a worktree and read every changed source/test file on the current branch, then verified each item against the team's PR standards — claims below are checked against the code, not the commit message.

✅ Verified fixed since my 08-18 review — all three 🔴 blockers are genuinely resolved

1. Regression test for the AST virtual-source skip — now present.
validators.test.ts:518 ("does not raise orphan findings when mapped AST nodes resolve to virtual/dependency/asset sources") maps AST nodes onto a webpack-runtime / node_modules / .png source set and asserts no orphan (SourceContentOrphanSources) and no unknown-ref findings surface (lines 544–547). Non-vacuous, and it directly guards the runAstChecks skip at source-content-verification.ts:273. Closes the gap I flagged on 08-18.

2. Performance / DRY cluster — resolved in code.

• Shared index, built once per target. engine.ts:129 builds buildSourceIndex(target.sourcePath) a single time and passes it into all three source validators (runOnTarget, lines 129–138). buildSourceIndex + INDEX_IGNORE_PREFIXES now live once in sourcemap-io.ts:9-27 — the verbatim copy-paste and the 3× tree walk are gone.
• Binary search. findNodeAtOffset is now a binary search over the document-ordered node array (source-content-verification.ts:416), and the byte-offset-sortedness invariant is documented at line 418 — which also closes @aruntyagiTutu's suggestion Bump semver from 6.3.0 to 6.3.1 #3.
• Per-mapping re-splits removed. structural-coherence precomputes submittedLines once (117-120); token-consistency precomputes compiledLines + linesCache once (127-131). classifyTokenAt/nameExistsNear/pointsToWhitespaceOrComment now operate on the cached string[] instead of split("\n") per sampled mapping.
• Residual (non-blocking): analyzeCoherence/analyzeTokenConsistency still eagerly split the whole shared index once per dist .js.map, including sources that file never references. Much smaller than the old O(N·M); a lazy/memoized split (or splitting inside buildSourceIndex) would finish it. Also — no large-project before/after numbers were posted; fine while the engine is out of default scans, but worth capturing before it's ever enabled by default.

3. coverage-analysis EOL over-credit — fixed.
analyzeCoverage now caps each mapping's reach at UNMAPPED_THRESHOLD and records inter-mapping gaps (coverage-analysis.ts:133-164, cap at line 151). New regression test at validators.test.ts:103 feeds a 5000-char line with a single column-0 mapping and asserts coveragePct < 50 with a non-empty unmapped region — the exact single-line-minified case that previously read ~100%.

Also still-good from earlier rounds: CRLF/CR normalize before byte-equal (source-content-verification.ts:213), POSIX-normalized index keys (sourcemap-io.ts:12), whitespace-ratio denominator tracking real sample fires (structural-coherence.ts:84-85), whitelist restored, severity Critical → High.

---

🟡 Still open — non-blocking; recommend folding into one GUS follow-up

4. Column convention is inconsistent and off-by-one. engine.toViolation adds +1 assuming validators emit 0-based columns (engine.ts:193), but:

• coverage-analysis.ts:63 emits startCol + 1 (already 1-based) → double-incremented, reports the column one too high;
• path-leakage.ts:24-25, missing-sourcemap.ts:30-31,46-47, invalid-source-references.ts:35-36 emit hardcoded startColumn: 1 → reported as column 2;
• only source-content-verification (raw 0-based) round-trips correctly.
  It survives review-by-test because every column assertion is just >= 1 (engine.test.ts:210, validators.test.ts:238) — none pins an exact column. Pick one convention (validators emit 0-based; engine converts).

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

6. Dead message-catalog entries + drift risk. NoBundleTargetsFound, SkippedForTarget, SkippedNoSourceTree (messages.ts:7-14) are unused; the engine hardcodes the same strings inline (engine.ts:72,123,150). Wire them up or delete them so the two copies can't drift.

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

8. Test temp dirs never cleaned up. makeTmpDir (test-helpers.ts:42-44) is called ~40× with no afterEach/afterAll anywhere; the docstring itself concedes cleanup is left to the caller. Accumulates in CI over time.

9. Missing error-path tests. VlqDecodingFailed (corrupt base64-VLQ decode, messages.ts:61) and the token-consistency malformed-.js.map path (the new TraceMap throw at token-consistency.ts:70-72) are untested. Segment-source-index-out-of-range and name-index-out-of-range are covered now — this is the remaining slice of the 81.7% branch vs 91–99% statement gap.

10. Whitelist has no in-code "remove once published" marker. validate-changed-package-versions.js:115-120 still lists packages/code-analyzer-uibundle-engine with no note — the revert is tracked only in this thread. Add // TODO: remove once @salesforce/code-analyzer-uibundle-engine is published (W-23659201). Related: identifyIncorrectlyVersionedPackages was not hardened — line 95 still calls semver.parse(releasedPackageVersion), which for an unpublished package resolves undefined → null → throw. The crash is only avoided by the whitelist entry, so the same trap returns for the next unpublished package that isn't whitelisted.

---

🟢 Nits

• Per-target Warn logs aren't aggregated (engine.ts:121-125,148-151) — consolidate per the logging standard.
• describeRules returns the shared mutable RULES array by reference (engine.ts:61) — return a copy.
• messages.ts uses "does not" / "do not" (lines 5, 134); team style prefers contractions.
• engine.test.ts rule cases (74–185) are it.each() candidates.

---

Bottom line

Every item I marked 🔴 on 08-18 is resolved in code, and items 1 and 3 are guarded by real regression tests — verified against the current head (f2aac4c), not just the commit message. Nothing here blocks merge anymore. Items 4–10 are Medium/Nit and, per our own severity model, are COMMENTED-class rather than CHANGES_REQUESTED.

Recommendation: clearing my standing CHANGES_REQUESTED — the blockers are cleared (companion CLI PR sequencing still applies). Roll 4–10 into a single follow-up under W-23659201; of those I'd prioritize #4 (users see a wrong column) and #10 (version-check hardening).

Generated via the code-analyzer-review-agent skill (standards from 604 review comments across 339 merged PRs), verified by reading the current branch head.

[aruntyagiTutu]

Reviewed the latest commit (2147a6b) addressing follow-up feedback. Most of it is solid — the message-catalog wiring for NoBundleTargetsFound/SkippedNoSourceTree/SkippedForTarget, the SourcemapNotValidJson surfacing from collectSourceMaps's new parseErrors return, the installTmpDirCleanup() afterEach pattern, the it.each(perRuleViolationCases) test consolidation, describeRules returning [...RULES] defensively, the findAncestorNamed rename, the path-leakage root expansion, and the contraction fixes in messages.ts all look correct and well-executed.

One regression, though: the startColumn 0-based change is backwards. The commit message claims this now "matches SFCA convention," but SFCA's convention is 1-based, not 0-based:

• packages/code-analyzer-regex-engine/src/engine.ts's getColumnNumber returns charIndex + 1 for line 1 (1-based).
• packages/code-analyzer-eslint-engine/src/engine.ts's normalizeStartValue does Math.max(startValue, 1) — enforces a 1-based floor.
• sfdx-code-analyzer-vscode/src/lib/diagnostics.ts's normalizeLocation defaults a missing startColumn to 1, and adjustToZeroBased explicitly subtracts 1 when converting a CodeLocation into a vscode.Position — i.e. the VS Code extension (the primary consumer of CodeLocation) treats SFCA's startColumn as 1-based and does its own 0-based conversion for the editor.

So the four validators that were changed from startColumn: 1 (or region.startCol + 1) to startColumn: 0 (or region.startCol) in coverage-analysis.ts, invalid-source-references.ts, missing-sourcemap.ts, and path-leakage.ts are now emitting 0-based columns, which will render one character off in VS Code and is inconsistent with every other engine in the repo.

Notably, this wasn't caught by CI because the existing "1-based coordinates" assertion in validators.test.ts:239 (expect(v.codeLocations[0]!.startColumn).toBeGreaterThanOrEqual(1)) uses a fixture with a valid sourcemap and an existing source file, so it never exercises the specific finding types (SourceFileDoesNotExist, MissingSourcemapForFile, OrphanJsWithDangerousApi, PathLeakageFinding, or a real CoverageUnmappedRegion) that hit the now-broken 0-based paths.

Suggest reverting these four back to 1-based (region.startCol + 1 in coverage-analysis.ts, startColumn: 1 in the other three), and ideally strengthening the E2E test fixture to actually trigger at least one of these finding types so a future regression like this fails CI.

[aruntyagiTutu]

Verified this commit against my prior CHANGES_REQUESTED on the startColumn convention.

On closer inspection, the previous behavior was already correct at the customer-facing level: engine.ts's toViolation() had Math.max(1, rawCol + 1), which converted the validators' 0-based ValidatorFinding.startColumn into a correctly 1-based CodeLocation.startColumn before it ever reached a consumer. My earlier flag compared the intermediate validator-level literal against SFCA's 1-based convention without tracing it through that conversion layer — that was my error, not a real bug in the prior commit.

That said, this commit is still a solid improvement:

• Moves 1-based emission directly into each validator (coverage-analysis.ts, invalid-source-references.ts, missing-sourcemap.ts, path-leakage.ts, source-content-verification.ts) instead of relying on a central compensating +1 in toViolation() — this matches how the regex/eslint engines emit literal 1-based values directly, which is more consistent and self-documenting.
• toViolation() now does Math.max(1, rawCol), a straightforward pass-through-with-floor instead of an offset — removes a subtle "you must remember validators are 0-based" trap for future contributors.
• New regression tests assert the exact value (startColumn === 1) rather than just >= 1 for missing-sourcemap, path-leakage, invalid-source-references, and coverage-analysis — meaningfully stronger than the previous loose bound, and would catch a future off-by-one in either direction.

Traced through each changed validator and confirmed the arithmetic is consistent: region.startCol + 1, literal 1, and node.column + 1 (Babel's 0-based loc.start.column) all now produce correct 1-based output given toViolation's updated pass-through.

No concerns with the rest of the diff (comment trimming in test-helpers.ts/engine.test.ts is a reasonable cleanup, not a loss of anything load-bearing).

[nikhil-mittal-165]

Automated re-review — post-8f0ed18 (Code Analyzer team standards)

Continuing this review thread. Since my last pass (post-f2aac4c, 08-19 08:52) two more commits landed — 2147a6b ("follow-up review comments") and 8f0ed18 ("align columns to 1-based") — which target exactly the Medium/Nit items I'd left open. Re-reviewed the current head (8f0ed18) the same way as before: checked out the branch, read every changed source/test file, and verified each item against the code, not the commit message. Also built + ran the suite locally on Node 20.20.2 (matching the @types/node@^20 pin): tsc --build clean, 73/73 tests pass across 4 suites (up from 69 — the later commits added regression tests).

---

✅ Verified resolved since my last review — items 4–10 + nits are all genuinely fixed in code

• Bump word-wrap from 1.2.3 to 1.2.4 #4 Column convention — fixed, and now guarded by an exact-column test. Every validator emits 1-based columns directly (coverage-analysis.ts:63 startCol + 1; source-content-verification.ts:313,342 node.column + 1; literal 1 in path-leakage/missing-sourcemap/invalid-source-references), and engine.toViolation is now a pass-through floor (Math.max(1, rawCol), engine.ts:191) instead of a compensating +1. The double-increment on coverage-analysis and the off-by-one startColumn: 1 → col 2 on the others are gone. New E2E test asserts the exact value startColumn === 1 for whole-file findings (engine.test.ts:180-216) — the loose >= 1 bound that let this hide is replaced. (Consistent with @aruntyagiTutu's trace-through in the 14:11 approval.)
• Updating contribution guidelines. #5 path-leakage roots — fixed. ABSOLUTE_UNIX_ROOT now covers /app, /build, /opt, /tmp, /var in addition to Users|home|root (path-leakage.ts:7) — the CI/Docker roots the rule exists to catch.
• Bump debug from 4.1.1 to 4.3.4 #6 Dead catalog entries — fixed. NoBundleTargetsFound / SkippedForTarget / SkippedNoSourceTree are wired through getMessage (engine.ts:71,121,148); no more hardcoded inline copies. Grepped the whole catalog — every key is now referenced exactly once outside messages.ts, so there are zero dead entries.
• Bump @babel/traverse from 7.15.4 to 7.23.2 #7 Malformed-JSON no longer silently dropped. collectSourceMaps returns parseErrors (sourcemap-io.ts:54-77), and both path-leakage and invalid-source-references surface them as SourcemapNotValidJson findings — so a malformed map is reported even if vlq-integrity is deselected. Covered by validators-integration.test.ts:126.
• NEW: @W-15652620@ - Prepare repo with standard/required files #8 Tmp-dir cleanup — fixed. installTmpDirCleanup() (an afterEach that rms every makeTmpDir()) is wired into all three suites (engine.test.ts:19, validators.test.ts:33, validators-integration.test.ts:25).
• NEW: @W-15652642@ - Add in starting files to prepare monorepo #9 Error-path tests — mostly closed. New tests cover segment-source-index-OOR, name-index-OOR (AICAC), missing sources, missing mappings, non-JSON maps, and — the specific one I'd flagged — the token-consistency unloadable-map path (validators-integration.test.ts:280). See residual Bump semver from 6.3.0 to 6.3.1 #3 below for the one branch still uncovered.
• NEW: @W-15652656@: Add public interfaces before adding any business logic #10 Whitelist marker — fixed. // TODO: remove once @salesforce/code-analyzer-uibundle-engine is published to npm (W-23659201) now sits on the entry (validate-changed-package-versions.js:118).
• Nits — fixed: describeRules returns [...RULES] (engine.ts:62); idx → lastNameSegmentIdx (engine.ts:211); the four per-rule violation cases are now it.each(perRuleViolationCases) (engine.test.ts:153); does not/do not replaced with contractions in messages.ts.

Also re-confirmed the earlier-round fixes still hold on this head: shared buildSourceIndex built once per target (engine.ts:127), binary-search findNodeAtOffset + documented sortedness invariant (source-content-verification.ts:416-420), coverage over-credit cap (coverage-analysis.ts:151), CRLF normalize before byte-equal (source-content-verification.ts:213), whitespace-ratio denominator (structural-coherence.ts:84-85), and the AST virtual-source regression test (validators-integration.test.ts:543).

---

🟡 Still open — all non-blocking; recommend folding into the W-23659201 follow-up

1. Perf residual — per-.js.map eager line-splitting of the whole source index. The 3× tree-walk is gone, but analyzeCoherence (structural-coherence.ts:117-120) and analyzeTokenConsistency (token-consistency.ts:129-131) still .split("\n") every file in the shared index once per compiled .js.map, including sources that map never references. For M compiled files × N sources that's still M×N splits. Cheap finish: split lazily on first use, or have buildSourceIndex cache string[] alongside the raw string. Much smaller than before — flagging so it isn't lost.
2. No large-project measurement. Per our own "measure before/after on very large projects with lots of violations" standard, this AST-heavy path (Babel-parse every compiled file + every mapped source) still has no posted numbers. Fine while the engine is out of default scans (thanks for pulling it) — but that's the gate before it's ever switched on by default.
3. VlqDecodingFailed branch untested (vlq-integrity.ts:61-67). Every other vlq-integrity branch now has a test; the decode() throw path doesn't. Note @jridgewell/sourcemap-codec.decode() is lenient and rarely throws — this may be effectively unreachable defensive code, in which case either add a test that triggers it or drop the branch.
4. identifyIncorrectlyVersionedPackages still not hardened. The TODO marker is in, but the underlying trap remains: semver.lte(semver.parse(version), semver.parse(releasedPackageVersion)) (validate-changed-package-versions.js:95) throws when releasedPackageVersion is undefined (unpublished). Only the whitelist entry prevents the crash — the next unpublished, non-whitelisted package hits it again. Optional: treat an unresolvable released version as "skip".

---

🟢 Nits (leftover)

• classification.ts:68-69 still builds "eval("/"Function(" via ["ev","al","("].join("") with no comment — this was @aruntyagiTutu's suggestion Bump word-wrap from 1.2.3 to 1.2.4 #4 from 08-17. One line noting it's to avoid self-tripping static scanners would save the next reader.
• coverage-analysis.ts:60 message text prints 0-based cols %d..%d while the structured startColumn is 1-based (region.startCol + 1) — a one-off between the human text and the location field. Cosmetic.
• Per-target Warn logs are still emitted one-per-rule in a loop (engine.ts:118-123) rather than aggregated — minor, per the "consolidate repetitive logs" standard.
• messages.ts:53,71,77 still use is not valid JSON / is not present — could be isn't for full contraction consistency; trivial.

---

Compatibility / hygiene — clean

@types/node pinned at ^20.0.0; @babel/* at ^7.25.0 (minor/patch); package version 0.1.0-SNAPSHOT. Goldfile severities are internally consistent (source-content-verification at High/2 after the Critical→High drop; the three informational rules at Info/5). Built + full suite green locally on Node 20.

---

Bottom line

Every item I marked 🔴 across the earlier rounds is fixed and regression-tested, and every 🟡/nit from my last pass is now resolved in code — verified against 8f0ed18, not the commit messages. Under our severity model the four remaining items are all COMMENTED-class, not CHANGES_REQUESTED, and two other reviewers (@aruntyagiTutu, @ankitsinghkuntal09) have approved.

I'm leaving my formal review state as-is for now rather than auto-clearing it — but from a standards standpoint there are no blockers left on this branch. Suggested follow-up under W-23659201: prioritize the perf residual (#1) + a large-project before/after measurement (#2) as the gate before this engine is ever enabled in a default scan; #3/#4 are cheap cleanups. Companion CLI PR (forcedotcom/code-analyzer#2080) sequencing still applies — this engine must publish before that one can go green.

Generated via the code-analyzer-review-agent standards (604 review comments across 339 merged PRs), verified by reading and building the current branch head.

[amritmishra-sf]

1. Perf residual — per-.js.map eager line-splitting of the whole source index. The 3× tree-walk is gone, but analyzeCoherence (structural-coherence.ts:117-120) and analyzeTokenConsistency (token-consistency.ts:129-131) still .split("\n") every file in the shared index once per compiled .js.map, including sources that map never references. For M compiled files × N sources that's still M×N splits. Cheap finish: split lazily on first use, or have buildSourceIndex cache string[] alongside the raw string. Much smaller than before — flagging so it isn't lost.
2. No large-project measurement. Per our own "measure before/after on very large projects with lots of violations" standard, this AST-heavy path (Babel-parse every compiled file + every mapped source) still has no posted numbers. Fine while the engine is out of default scans (thanks for pulling it) — but that's the gate before it's ever switched on by default.
3. VlqDecodingFailed branch untested (vlq-integrity.ts:61-67). Every other vlq-integrity branch now has a test; the decode() throw path doesn't. Note @jridgewell/sourcemap-codec.decode() is lenient and rarely throws — this may be effectively unreachable defensive code, in which case either add a test that triggers it or drop the branch.
4. identifyIncorrectlyVersionedPackages still not hardened. The TODO marker is in, but the underlying trap remains: semver.lte(semver.parse(version), semver.parse(releasedPackageVersion)) (validate-changed-package-versions.js:95) throws when releasedPackageVersion is undefined (unpublished). Only the whitelist entry prevents the crash — the next unpublished, non-whitelisted package hits it again. Optional: treat an unresolvable released version as "skip".

Perf residual can be skipped for now, as it's from a generated file and the VlqDecodingFailed branch untested we will revisit later if needed

[nikhil-mittal-165]

@amritmishra-sf

Can you help me explain
Perf residual can be skipped for now, as it's from a generated file

I believe only the source map is generated the source files are still hand written right ?
the developer's src/ tree — and that's hand-written source the developer fully controls and can grow arbitrarily large.

[ankitsinghkuntal09]

Done W-23659201-ReSanity-8f0ed18-evidenceDoc: https://docs.google.com/document/d/1hvPWOkNMWY12X8uiXKyB5F93DgQ1UjCs/edit?usp=sharing&ouid=112790920304594839224&rtpof=true&sd=true

[nikhil-mittal-165]

Approving this PR , there are some performance issues which will be addressed before the engine comes in as a default engine

[nikhil-mittal-165]

sf code-analyzer run --rule-selector all will invoke the engine we dont want that
refer this

code-analyzer-core/packages/code-analyzer-core/src/rules.ts

Line 112 in a445e36

const isDevPreviewApexGuru: boolean = tags.includes('devpreviewapexguru');

to avoid it

[amritmishra-sf]

sf code-analyzer run --rule-selector all will invoke the engine we dont want that refer this

code-analyzer-core/packages/code-analyzer-core/src/rules.ts

Line 112 in a445e36

const isDevPreviewApexGuru: boolean = tags.includes('devpreviewapexguru');

to avoid it

One option that we are looking at is adding a similar UIBundle check in the same place, to restrict it
const isDevPreviewUIBundle: boolean = tags.includes('uibundle');

[nikhil-mittal-165]

sf code-analyzer run --rule-selector all will invoke the engine we dont want that refer this

code-analyzer-core/packages/code-analyzer-core/src/rules.ts

Line 112 in a445e36

const isDevPreviewApexGuru: boolean = tags.includes('devpreviewapexguru');

to avoid it

One option that we are looking at is adding a similar UIBundle check in the same place, to restrict it const isDevPreviewUIBundle: boolean = tags.includes('uibundle');

yes this makes sense

[amritmishra-sf]

@amritmishra-sf

Can you help me explain Perf residual can be skipped for now, as it's from a generated file

I believe only the source map is generated the source files are still hand written right ? the developer's src/ tree — and that's hand-written source the developer fully controls and can grow arbitrarily large.

Yes, the map is generated, and the source could grow over time. We’d definitely like to revisit the performance aspect once the initial changes are in and we’ve reached a functionally ready state.

[nikhil-mittal-165]

Automated re-review — post-6fa8155 (Code Analyzer team standards)

Continuing this thread. Since my last pass (post-8f0ed18, 05:45 today) two commits landed — b2d2d32 ("reduce Info-tier false positives") and 6fa8155 ("make UIBundle rules opt-in for rule-selector all"), plus a dev merge (2266307). This pass statically re-reviewed both at the current head: read every changed source/test file and hand-traced the coverage arithmetic. (I didn't re-run the suite this pass — relying on green CI plus the BACKSTOP tests I read below.) As before, verified against the code, not the commit messages.

---

✅ 6fa8155 — the --rule-selector all opt-in you raised this morning is in, correct, and tested

• This is exactly the fix from the thread you opened this morning ("--rule-selector all will invoke the engine we don't want"). matchesRuleSelector (packages/code-analyzer-core/src/rules.ts:108-135) now puts UIBundle (and DevPreviewApexGuru) rules into an opt-in selectables set of engine name / rule name / explicit tag only — all, severity number, and severity name are excluded. So sf code-analyzer run --rule-selector all (and 4 / Low) won't pull the engine in; only uibundle, a rule name, UIBundle, or UIBundleIntegrity will.
• Correctness detail I checked: the match is case-insensitive — tags.map(t => t.toLowerCase()) then tags.includes('uibundle') (rules.ts:111-113), and the engine tags its rules "UIBundle" (uibundle-engine/src/rules.ts:7). The lowercasing makes the capitalized real tag match the lowercase check, so there's no casing leak into all.
• Tested comprehensively — test/rule-selection.test.ts:655-695 adds a dedicated UIBundle rule selection behavior block (mirrors the DevPreviewApexGuru one): NOT selected by severity number / severity name / all; ARE selected by engine name / rule name / UIBundle tag / UIBundleIntegrity tag. That's the full opt-in contract.
• This also closes my prior 🟡 Bump tough-cookie from 4.0.0 to 4.1.3 #2 the cleaner way: I'd flagged "large-project measurement is the gate before this is ever in a default scan." It now structurally can't be in a default (all) scan, so that risk is removed for the common path.

✅ b2d2d32 — Info-tier false-positive reductions verified

JSX synthetic-name suppression + namespace-prefix-match suppression are in and regression-tested (confirmed last pass). Constants were raised to cut Info noise on clean production bundles: LINE1_EXEMPT_CHARS 150→2000, EXCESSIVE_UNMAPPED_PCT 2→5, plus dense-line thresholds (coverage-analysis.ts:9-17). The over-credit cap and dense-line handling still hold, and the cumulative-budget signal remains exercised by the BACKSTOP tests (validators.test.ts:501-547, which assert excessiveUnmapped both true and false).

---

🟡 New — one doc/code drift introduced by b2d2d32 (Low, non-blocking)

The constant bump didn't propagate to the customer-facing rule description. messages.ts:32 (snapshotted into test-data/uibundle-engine-goldfile.json:65) still reads:

"…raises a cumulative finding when more than 2% of the compiled file (line-1 preamble discounted up to 150 chars) has no sourcemap coverage."

…but the code is EXCESSIVE_UNMAPPED_PCT = 5.0 and LINE1_EXEMPT_CHARS = 2000. It's also self-contradictory: the finding text itself (CoverageExcessiveCumulative, messages.ts:112-113) correctly prints "Threshold is 5%", so a user reading the rule description sees "2%" while the actual finding says "5%". Fix: update messages.ts:32 to "5%" / "2000 chars" and regenerate the goldfile. (Standards #8 docs-match-code / #11.)

🟢 Nits

• Stale test labels from the same bump. validators.test.ts:96 is titled "exempts up to 150 chars on line 1" (now 2000; the test uses 100 chars, so it still passes — just mislabeled). More notably, validators-integration.test.ts:48 — "emits an unmapped-region finding and cumulative-budget finding" (comment: "line 1 has ~200 chars, >150 exempt") — no longer triggers the cumulative finding: under the 2000-char line-1 exemption a 200-char region contributes 0 to the budget (coverage-analysis.ts:209-213), so excessiveUnmapped=false and only the per-region finding fires. The test still passes because it only asserts findings.length > 0. The excessive path stays covered by the BACKSTOP tests, so this is naming/intent drift, not a coverage hole — worth renaming or bumping the fixture past 2000 chars so it tests what its name claims.
• Prior leftover nits unchanged: classification.ts:68-69 split-string eval(/Function( (no comment); coverage-analysis.ts prints 0-based cols in message text while startColumn is 1-based; per-rule Warn loop in engine.ts:118-123 not aggregated; VlqDecodingFailed branch (vlq-integrity.ts:61-67) still untested (likely-unreachable defensive code — add a triggering test or drop the branch).

Perf residual — acknowledged as deferred

My prior 🟡 #1 (per-.js.map eager line-splitting of the whole source index in structural-coherence / token-consistency) is unchanged in code — but per this morning's thread the team has agreed to revisit performance "once … we've reached a functionally ready state," and the source tree is hand-written/developer-controlled. Agreed: follow-up under W-23659201, not a blocker here.

---

Bottom line

The two commits since my last pass resolve the final substantive item — the --rule-selector all opt-in you raised this morning — and it's correctly implemented (case-insensitive tag match, no leak) and well-tested. The only new actionable is the coverage-analysis rule-description drift (Low) plus a test-label nit; both are quick, non-blocking cleanups. Nothing here is CHANGES_REQUESTED-class, consistent with the PR's current APPROVED state (incl. @aruntyagiTutu, @ankitsinghkuntal09, and my own approval at 06:56).

Suggested tidy-ups to fold into the W-23659201 follow-up: (1) fix the messages.ts 2%/150 → 5%/2000 drift and regenerate the goldfile; (2) rename or re-fixture the two stale coverage tests; (3) the perf residual + a large-project before/after measurement remain the gate before this engine is ever enabled in a default scan.

Generated via the code-analyzer-review-agent standards, verified by reading and hand-tracing the current branch head (6fa8155).

[jag-j]

@amritmishra-sf @nikhil-mittal-165 Here and in other such checks in other files should .mjs/.cjs files also be included?

[amritmishra-sf]

Hi @jag-j, we’re trying to use this tool to perform integrity checks for UI Bundles, which include a dist directory in the packaged code that gets installed in the subscriber org. So yes, these files are important for our use case at the moment.

[jag-j]

@amritmishra-sf Are you saying you will make the change to include cjs, mjs files?

[amritmishra-sf]

yes, ideally these should be. I think it was missed in the initial changes. Thanks @jag-j for pointing that out. I will push the changes for those two as well

[nikhil-mittal-165]

Automated re-review — post-78bb83f (Code Analyzer team standards)

Continuing the thread. Since my last pass (post-6fa8155, 08-20) three commits landed, none of which had been reviewed yet:

• 1807c2f — cover .mjs/.cjs compiled bundles (from @jag-j's coverage-analysis.ts:44 question)
• 9273f88 — align coverage-analysis docs/tests to the 5% / 2000-char constants
• 78bb83f — close three tamper-detection bypasses

Method: fetched the PR head (78bb83f) into a worktree and read every changed source/test file, hand-tracing the tamper-detection paths. Verified against the code, not the commit messages. (Relied on green CI for execution — this monorepo worktree has no node_modules; every test cited below was read to confirm it's non-vacuous.)

---

✅ All three new commits are correct and tested

1807c2f — .mjs/.cjs coverage is complete and consistent. The extension check is centralized in classification.ts (isCompiledJs accepts .js/.mjs/.cjs; isSourcemap accepts the three .map counterparts) and all six file-walking validators + sourcemap-io now route through it. The ${jsPath}.map derivation stays correct for all three (main.mjs → main.mjs.map), and isCompiledJs/isSourcemap are disjoint (a .js.map ends in .map, so it's never misread as compiled JS). Covered end-to-end (engine.test.ts:285-327, it.each over mjs/cjs for missing-sourcemap + path-leakage) and at unit level (validators.test.ts:66-76). This closes the real gap @jag-j raised — a tampered .mjs/.cjs bundle would previously have been skipped by every detector.

9273f88 — closes the exact doc/test drift I flagged post-6fa8155. messages.ts:32 and the goldfile (uibundle-engine-goldfile.json:65) now read "more than 5% … up to 2000 chars", matching EXCESSIVE_UNMAPPED_PCT = 5.0 / LINE1_EXEMPT_CHARS = 2000 and the finding text ("Threshold is 5%"). The self-contradiction is gone. The two stale-fixture tests were re-pointed at the 2000-char path so they exercise what their names claim rather than passing vacuously.

78bb83f — the three bypass fixes are sound and adversarially tested.

• missing-sourcemap comment-only bypass — hasSourcemap no longer accepts a bare //# sourceMappingURL comment as proof: it rejects data: and remote scheme:// URLs, resolves the URL relative to the .js, and requires the target to be inside distPath, satisfy isSourcemap(), and be a regular file. Tested with non-vacuous assertions for nonexistent map, remote HTTP URL, data URL, non-sourcemap target (../README.md), self-reference, and outside-distPath (engine.test.ts:93-177).
• vlq-integrity .mjs.map/.cjs.map — the last endsWith(".js.map") holdout now uses isSourcemap().
• isVirtualSource over-match — the "any ?" gate is replaced with a fixed allow-list of real Vite/webpack/Rollup query forms (classification.ts:21-33), so src/injected.js?x=1 is no longer laundered as virtual. Directly tested: ?x=1, ?tampered, ?tampered=1 → false; ?vue&type=script&lang.ts → true (validators.test.ts:57-59).

Prior-round fixes all still hold on this head: shared buildSourceIndex built once per target (engine.ts:127), binary-search findNodeAtOffset (source-content-verification.ts:423), coverage over-credit cap (coverage-analysis.ts:188), CRLF normalize before byte-equal (:220), whitespace-ratio denominator (structural-coherence.ts:84-85), 1-based columns (engine.ts:188), path-leakage CI/Docker roots (path-leakage.ts:7), catalog entries wired (engine.ts:71,121,148), TODO marker on the whitelist, and the AST virtual-source regression test.

---

🟡 New this pass — one issue (Medium, non-blocking): symlinked entries are handled inconsistently

hasSourcemap gates the colocated map on fs.stat(colocated).isFile() (missing-sourcemap.ts:77-78). fs.stat follows symlinks, so for a .map that is a symlink to a regular file, isFile() is true and the map is accepted. But every enumerator in the engine — collectJsFiles (missing-sourcemap.ts:63) and the shared walk (sourcemap-io.ts:88-90) — uses dirent.isFile(), which is false for a symlink (readdir doesn't resolve links). So:

• A symlinked .js.map is treated as "present" by missing-sourcemap (no finding) but silently skipped by the three walk-based validators that would inspect it — vlq-integrity, path-leakage, invalid-source-references. (The direct-readFile validators — source-content-verification, coverage-analysis, structural-coherence, token-consistency — do follow it, so it's scanned by some rules and not others.)
• More broadly, a symlinked compiled .js/.mjs/.cjs is enumerated by no validator (both collectJsFiles and walk skip it), so it isn't scanned at all and isn't even reported as missing a sourcemap.

The 78bb83f comment states the check "rejects … the isFile-false symlinks walk() would skip anyway" — that reasoning is inverted: fs.stat().isFile() is true for a symlink, not false, so this path does not reject them, and the semantics don't match walk. The commit added a directory test (engine.test.ts:146, where stat().isFile() correctly returns false) but no symlink test, so the gap is unguarded.

This isn't a merge blocker — its practical reach assumes an attacker who fully controls dist/, and the strongest rule (source-content-verification) still reads symlinked maps — but for a tamper detector, "silently skip all symlinks" is a real blind spot and the code comment is wrong. Suggested fix: decide a symlink policy explicitly — either lstat/isSymbolicLink() so hasSourcemap matches walk (treat symlinked map as absent → flag it), or resolve-and-scan in walk so symlinked files are covered — rather than the current split behavior. Add one symlink regression test alongside the directory one.

---

🟢 Still-open residuals (unchanged, non-blocking — recommend folding into the W-23659201 follow-up)

1. Perf residual — per-.js.map eager line-splitting of the whole source index. analyzeCoherence (structural-coherence.ts:117-120) and analyzeTokenConsistency (token-consistency.ts:135-139) still .split("\n") every file in the shared index once per compiled .js.map (M×N splits). Cheap finish: cache string[] in buildSourceIndex next to the raw string. Smaller than the pre-f2aac4c 3× tree-walk, but still there.
2. No large-project measurement. This Babel-parse-every-compiled-file path still has no posted before/after numbers on a project with thousands of files. Fine while the engine is out of default scans (thanks for the 6fa8155 opt-in) — but that's the gate before it's ever switched on by default. Per our own "measure on very large projects with lots of violations" standard.
3. VlqDecodingFailed branch still untested (vlq-integrity.ts:61-68). Every other vlq-integrity branch now has a test; decode()'s throw path doesn't (grep for VlqDecodingFailed hits only src). @jridgewell/sourcemap-codec.decode() is lenient and rarely throws — either add a triggering test or drop the branch as unreachable.
4. identifyIncorrectlyVersionedPackages still not hardened. The TODO marker is in, but semver.lte(semver.parse(v), semver.parse(releasedPackageVersion)) still throws when the released version is undefined (unpublished) — only the whitelist entry prevents the crash. The next unpublished, non-whitelisted package hits it again. Optional: treat an unresolvable released version as "skip".

Nits (leftover): classification.ts:98-99 still builds eval(/Function( via .join("") with no comment (@aruntyagiTutu's 08-17 #4); coverage-analysis.ts:72 prints 0-based cols %d..%d in the message text while startColumn is 1-based (:75); per-target Warn logs still emitted one-per-rule (engine.ts:118-123) rather than aggregated. All trivial. One residual worth a line in the follow-up: the isVirtualSource allow-list still skips a genuine ?raw/?url suffix on a mapped node, and mapped-to-virtual nodes bypass the dangerous-pattern backstop (which only scans unmapped nodes) — much narrower than the old "any ?", and partly backstopped by invalid-source-references + the 20% virtual-ratio cap, so low priority.

---

Compatibility / hygiene — clean

@types/node pinned at ^20.0.0; @babel/* at ^7.25.0 (minor/patch); package 0.1.0-SNAPSHOT. Goldfile has all 8 rules carrying both UIBundle (opt-in) and UIBundleIntegrity tags; severities internally consistent (source-content-verification at High/2 after the Critical→High drop; the three informational rules at Info/5). Commit headlines follow FIX @W-XXXXX@.

---

Bottom line

The three commits since my last pass are all correct: .mjs/.cjs coverage closes @jag-j's gap, the coverage doc-drift I flagged is fixed and re-tested, and the three tamper-detection bypass fixes are sound and adversarially tested. The only thing this pass surfaced is the symlink-handling inconsistency above — Medium, non-blocking, one-line fix + a test. Everything else is unchanged non-blocking residuals; under our severity model none is CHANGES_REQUESTED-class, consistent with the PR's approved state. Suggested follow-up gate before this engine is ever enabled in a default scan: the perf residual (#1) + a large-project before/after measurement (#2), plus the symlink policy decision. Companion CLI PR (forcedotcom/code-analyzer#2080) sequencing still applies — this engine must publish before that one can go green.

Generated via the code-analyzer-review-agent standards (604 review comments across 339 merged PRs), verified by reading and hand-tracing the current branch head (78bb83f).

[nikhil-mittal-165]

✅ Symlink fix (62cc1a6) confirmed — closes my finding from the last pass

Verified the fix directly against the code and the new test:

• Both .map file-ness checks in hasSourcemap are now fs.lstat instead of fs.stat (missing-sourcemap.ts:76 colocated, :98 resolved-URL target). lstat doesn't follow symlinks, so st.isFile() is false for a symlinked map — now matching the Dirent.isFile() semantics that collectJsFiles and sourcemap-io.walk use. A symlinked map is treated as absent and flagged, so missing-sourcemap and the walk-based validators (vlq-integrity, path-leakage, invalid-source-references) no longer disagree.
• The fs.stat still on distPath (:12) is correct to leave as-is — that's the dist directory root (the walk/readdir entry point), not an enumerated child entry, so following a symlinked root is the right behavior.
• The comment is corrected and now accurate ("lstat matches walker semantics — Dirent.isFile() is false for symlinks…").
• Regression test is present and non-vacuous (engine.test.ts:160): symlinks dist/main.js.map → dist/real.js.map, runs the rule, asserts a missing-sourcemap violation fires. Compiles cleanly (node:fs/node:path are imported).

That was the only new actionable from my last review, and it's fully resolved. Remaining items are the unchanged non-blocking residuals for the W-23659201 follow-up: perf line-splitting (structural-coherence.ts:117-120, token-consistency.ts:135-139), a large-project before/after measurement (the gate before default-scan enablement), the untested VlqDecodingFailed branch (vlq-integrity.ts:61-68), and the semver.parse(undefined) trap in the version-check script. None is a blocker.

Verified by reading the current branch head (62cc1a6) in a worktree.