Sync Windows hotfix to release/bi-1.8.x#1561
Sync Windows hotfix to release/bi-1.8.x#1561kanushka wants to merge 14 commits intorelease/bi-1.8.xfrom
Conversation
…g specific errors encountered during directory access.
…cy specifications for various packages
…r both mi-extension and mi-visualizer
… autoinstaller Add pnpm override to force fast-xml-parser to 5.3.7 in rush-plugins and regenerate lockfile using rush update-autoinstaller. Co-authored-by: Cursor <cursoragent@cursor.com>
[Hotfix] Enhance Windows environment detection in BallerinaExtension
|
|
📝 WalkthroughWalkthroughThis pull request addresses dependency version updates, CVE vulnerability tracking, and enhances Windows environment detection in the Ballerina extension. Fast-xml-parser is bumped to 5.3.7 across multiple package manifests, three CVEs are added to the ignore list, and the Ballerina extension is updated to v5.8.1 with improved Windows installation discovery and PATH environment handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
common/config/rush/.pnpmfile.cjs (1)
79-89: Consider consolidating duplicated override assignments into a shared helper map.This update is correct, but the same override edits are repeated in both branches, which increases drift risk over time.
♻️ Suggested refactor
+const FORCE_VERSIONS = { + 'fast-xml-parser': '5.3.7', + 'bn.js': '5.2.3' +}; + +function applyOverrides(depSet) { + if (!depSet) return; + for (const [name, version] of Object.entries(FORCE_VERSIONS)) { + if (depSet[name]) depSet[name] = version; + } +} + module.exports = { hooks: { readPackage(pkg, context) { - if (pkg.dependencies) { - ... - if (pkg.dependencies['fast-xml-parser']) { - pkg.dependencies['fast-xml-parser'] = '5.3.7'; - } - if (pkg.dependencies['bn.js']) { - pkg.dependencies['bn.js'] = '5.2.3'; - } - } + applyOverrides(pkg.dependencies); - if (pkg.devDependencies) { - ... - if (pkg.devDependencies['fast-xml-parser']) { - pkg.devDependencies['fast-xml-parser'] = '5.3.7'; - } - if (pkg.devDependencies['bn.js']) { - pkg.devDependencies['bn.js'] = '5.2.3'; - } - } + applyOverrides(pkg.devDependencies);Also applies to: 146-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/config/rush/.pnpmfile.cjs` around lines 79 - 89, The duplicate dependency override assignments for pkg.dependencies keys ('fast-xml-parser', 'hono', 'lodash', 'bn.js') are repeated in multiple branches; extract a single helper map (e.g., const dependencyOverrides = { 'fast-xml-parser': '5.3.7', 'hono': '^4.11.7', 'lodash': '4.17.23', 'bn.js': '5.2.3' }) and replace the repeated blocks in the .pnpmfile.cjs logic (the sections that currently check pkg.dependencies[...] and assign values, including the other duplicate at lines ~146-156) with a small loop or function that iterates dependencyOverrides and applies pkg.dependencies[key] = value when present. Ensure you reference and reuse this helper in both branches instead of duplicating assignments.workspaces/ballerina/ballerina-extension/src/core/extension.ts (1)
2694-2769:findWindowsBallerinaPath— Strategy 2 iterates files unnecessarilyMinor observation:
fs.readdirSync(root)(Line 2753) returns both files and directories. For each child (including.exe,.txt, licence files, etc. that may live at the Ballerina install root), the code constructspath.join(root, child, 'bin')and callsfs.existsSync. Adding anisDirectoryguard avoids needless probes:♻️ Suggested refinement
const children = fs.readdirSync(root); for (const child of children) { + const childPath = path.join(root, child); + try { + if (!fs.statSync(childPath).isDirectory()) { continue; } + } catch { continue; } const versionedBin = path.join(root, child, 'bin'); if (fs.existsSync(path.join(versionedBin, 'bal.bat'))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts` around lines 2694 - 2769, The loop in findWindowsBallerinaPath currently calls fs.readdirSync(root) and treats every child as if it were a directory, causing unnecessary fs.existsSync probes for files; change the inner loop to skip non-directories by checking each child with fs.statSync or fs.lstatSync(rootChild).isDirectory() (wrap the stat call in try/catch to skip unreadable entries) before constructing versionedBin and testing for bal.bat, so only real subdirectories are probed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.trivyignore:
- Around line 8-15: Update the three new CVE entries in .trivyignore
(CVE-2025-14505, CVE-2025-69873, CVE-2026-26996) to include a GitHub issue
tracking link like the existing CVE-2020-36851 entry; create or reference an
appropriate repo issue for each CVE and append the issue URL to each CVE comment
so every ignored CVE has a traceable GitHub issue for governance and periodic
re-evaluation.
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2721-2727: findWindowsBallerinaPath currently uses synchronous
fs.existsSync(path.join(entry, 'bal.bat')) for every PATH entry (called
synchronously from getBallerinaVersion), which can block the extension host on
slow or network-mounted PATH entries; update the function to avoid synchronous
blocking by either (a) pre-filtering entries to skip UNC/network paths (entries
starting with '\\\\') and first guard-checking that the entry is a local
directory via a cheap sync test like fs.statSync(entry).isDirectory() before
probing for bal.bat, or (b) convert the search to an asynchronous implementation
using fs.promises.stat/access and make getBallerinaVersion await the async probe
(or run it on a worker), and add a brief comment documenting the performance
expectation; reference function names findWindowsBallerinaPath,
getBallerinaVersion, and the use of fs.existsSync/path.join when making changes.
- Around line 2785-2792: The PowerShell snippet assigned to the variable
"command" currently replaces the process-level Path with only Machine+User
registry values, dropping VS Code-injected entries; update that snippet to
preserve the current process PATH by capturing it (e.g.,
$p=[Environment]::GetEnvironmentVariable('Path','Process') or using $e['Path'])
and append it when building $e['Path'] (set $e['Path']=$mp+';'+$up+';'+$p while
handling empty mp/up/p cases) so when updateProcessEnv writes process.env.Path
the VS Code entries remain and registry paths are prepended.
---
Nitpick comments:
In `@common/config/rush/.pnpmfile.cjs`:
- Around line 79-89: The duplicate dependency override assignments for
pkg.dependencies keys ('fast-xml-parser', 'hono', 'lodash', 'bn.js') are
repeated in multiple branches; extract a single helper map (e.g., const
dependencyOverrides = { 'fast-xml-parser': '5.3.7', 'hono': '^4.11.7', 'lodash':
'4.17.23', 'bn.js': '5.2.3' }) and replace the repeated blocks in the
.pnpmfile.cjs logic (the sections that currently check pkg.dependencies[...] and
assign values, including the other duplicate at lines ~146-156) with a small
loop or function that iterates dependencyOverrides and applies
pkg.dependencies[key] = value when present. Ensure you reference and reuse this
helper in both branches instead of duplicating assignments.
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2694-2769: The loop in findWindowsBallerinaPath currently calls
fs.readdirSync(root) and treats every child as if it were a directory, causing
unnecessary fs.existsSync probes for files; change the inner loop to skip
non-directories by checking each child with fs.statSync or
fs.lstatSync(rootChild).isDirectory() (wrap the stat call in try/catch to skip
unreadable entries) before constructing versionedBin and testing for bal.bat, so
only real subdirectories are probed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
common/autoinstallers/rush-plugins/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcommon/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.trivyignorecommon/autoinstallers/rush-plugins/package.jsoncommon/config/rush/.pnpmfile.cjspackage.jsonworkspaces/ballerina/ballerina-extension/CHANGELOG.mdworkspaces/ballerina/ballerina-extension/package.jsonworkspaces/ballerina/ballerina-extension/src/core/extension.tsworkspaces/mi/mi-extension/package.jsonworkspaces/mi/mi-visualizer/package.json
| # No fix released by the author | ||
| CVE-2025-14505 | ||
|
|
||
| # Library is used in nested dependencies and not directly used by our codebase. No fix released by the author. | ||
| CVE-2025-69873 | ||
|
|
||
| # used in nested dependencies and not directly used by our codebase. No fix released by the author. | ||
| CVE-2026-26996 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .trivyignoreRepository: wso2/vscode-extensions
Length of output: 656
🏁 Script executed:
rg -i 'github\.com.*issue|tracking|reviewer|date|expires' .trivyignoreRepository: wso2/vscode-extensions
Length of output: 120
🏁 Script executed:
# Search the entire codebase for these CVE IDs to see if there are tracking references elsewhere
rg 'CVE-2025-14505|CVE-2025-69873|CVE-2026-26996' --type markdown --type yaml --type jsonRepository: wso2/vscode-extensions
Length of output: 48
Add GitHub issue links to each newly ignored CVE.
The existing suppression (CVE-2020-36851, line 5–6) includes a tracking link (https://github.com/wso2/vscode-extensions/issues/550). The three new CVE entries (CVE-2025-14505, CVE-2025-69873, CVE-2026-26996) lack issue references, making them permanent suppressions without traceability or expiry. Follow the established pattern and include a GitHub issue link for each new entry to maintain governance and enable periodic re-evaluation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.trivyignore around lines 8 - 15, Update the three new CVE entries in
.trivyignore (CVE-2025-14505, CVE-2025-69873, CVE-2026-26996) to include a
GitHub issue tracking link like the existing CVE-2020-36851 entry; create or
reference an appropriate repo issue for each CVE and append the issue URL to
each CVE comment so every ignored CVE has a traceable GitHub issue for
governance and periodic re-evaluation.
| for (const entry of pathEntries) { | ||
| const candidate = path.join(entry, 'bal.bat'); | ||
| if (fs.existsSync(candidate)) { | ||
| debug(`[WIN_BAL_FIND] Found bal.bat in registry PATH entry: ${entry}`); | ||
| return entry + path.sep; | ||
| } | ||
| } |
There was a problem hiding this comment.
Synchronous fs.existsSync across all PATH entries may stall the extension host
Strategy 1 calls fs.existsSync(path.join(entry, 'bal.bat')) synchronously for every entry in the combined Machine+User PATH. On typical Windows developer machines this can be 80–200+ entries; many corporate environments add UNC (\\server\share\...) or mapped-drive entries that incur network round-trips for each stat call. Since findWindowsBallerinaPath runs on the extension host thread (called synchronously from the else if branch in getBallerinaVersion), a single slow network PATH entry can block activation for several seconds per probe.
Consider failing fast by only probing entries that are local (e.g., skip entries starting with \\ or that don't fs.existsSync as a directory first with a short guard), or move the search to an async worker. At minimum, document the performance expectation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts` around lines
2721 - 2727, findWindowsBallerinaPath currently uses synchronous
fs.existsSync(path.join(entry, 'bal.bat')) for every PATH entry (called
synchronously from getBallerinaVersion), which can block the extension host on
slow or network-mounted PATH entries; update the function to avoid synchronous
blocking by either (a) pre-filtering entries to skip UNC/network paths (entries
starting with '\\\\') and first guard-checking that the entry is a local
directory via a cheap sync test like fs.statSync(entry).isDirectory() before
probing for bal.bat, or (b) convert the search to an asynchronous implementation
using fs.promises.stat/access and make getBallerinaVersion await the async probe
(or run it on a worker), and add a brief comment documenting the performance
expectation; reference function names findWindowsBallerinaPath,
getBallerinaVersion, and the use of fs.existsSync/path.join when making changes.
| command = 'powershell.exe -NoProfile -Command "' + | ||
| '$e=[Environment]::GetEnvironmentVariables(\'Process\');' + | ||
| '$mp=[Environment]::GetEnvironmentVariable(\'Path\',\'Machine\');' + | ||
| '$up=[Environment]::GetEnvironmentVariable(\'Path\',\'User\');' + | ||
| 'if($mp -and $up){$e[\'Path\']=$mp+\';\'+$up}' + | ||
| 'elseif($mp){$e[\'Path\']=$mp}' + | ||
| 'elseif($up){$e[\'Path\']=$up};' + | ||
| '$e | ConvertTo-Json"'; |
There was a problem hiding this comment.
Windows PATH merge drops VS Code's process-level PATH entries
$e=[Environment]::GetEnvironmentVariables('Process') captures the full live process PATH (including entries VS Code injects at startup — its bundled Node.js, extension host paths, etc.), but the very next assignment $e['Path'] = $mp+';'+$up replaces it entirely with only the registry Machine+User values. After updateProcessEnv writes this back via process.env.Path = newEnv.Path (Line 2656), all subsequent exec calls that rely on those process-level injected entries can fail — not just Ballerina's. The comment says "VS Code-internal variables are preserved" but that applies only to non-Path keys.
To preserve the process-level entries, append the current process PATH instead of discarding it:
🔧 Proposed fix — include process PATH in the merge
- command = 'powershell.exe -NoProfile -Command "' +
- '$e=[Environment]::GetEnvironmentVariables(\'Process\');' +
- '$mp=[Environment]::GetEnvironmentVariable(\'Path\',\'Machine\');' +
- '$up=[Environment]::GetEnvironmentVariable(\'Path\',\'User\');' +
- 'if($mp -and $up){$e[\'Path\']=$mp+\';\'+$up}' +
- 'elseif($mp){$e[\'Path\']=$mp}' +
- 'elseif($up){$e[\'Path\']=$up};' +
- '$e | ConvertTo-Json"';
+ command = 'powershell.exe -NoProfile -Command "' +
+ '$e=[Environment]::GetEnvironmentVariables(\'Process\');' +
+ '$mp=[Environment]::GetEnvironmentVariable(\'Path\',\'Machine\');' +
+ '$up=[Environment]::GetEnvironmentVariable(\'Path\',\'User\');' +
+ '$pp=$e[\'Path\'];' +
+ '$merged=@($mp,$up,$pp | Where-Object{$_}) -join \';\';' +
+ '$e[\'Path\']=$merged;' +
+ '$e | ConvertTo-Json"';This ensures the registry paths (where a fresh Ballerina install writes) are prepended so that bal.bat is found first, while VS Code's own process entries are still present for everything else.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command = 'powershell.exe -NoProfile -Command "' + | |
| '$e=[Environment]::GetEnvironmentVariables(\'Process\');' + | |
| '$mp=[Environment]::GetEnvironmentVariable(\'Path\',\'Machine\');' + | |
| '$up=[Environment]::GetEnvironmentVariable(\'Path\',\'User\');' + | |
| 'if($mp -and $up){$e[\'Path\']=$mp+\';\'+$up}' + | |
| 'elseif($mp){$e[\'Path\']=$mp}' + | |
| 'elseif($up){$e[\'Path\']=$up};' + | |
| '$e | ConvertTo-Json"'; | |
| command = 'powershell.exe -NoProfile -Command "' + | |
| '$e=[Environment]::GetEnvironmentVariables(\'Process\');' + | |
| '$mp=[Environment]::GetEnvironmentVariable(\'Path\',\'Machine\');' + | |
| '$up=[Environment]::GetEnvironmentVariable(\'Path\',\'User\');' + | |
| '$pp=$e[\'Path\'];' + | |
| '$merged=@($mp,$up,$pp | Where-Object{$_}) -join \';\';' + | |
| '$e[\'Path\']=$merged;' + | |
| '$e | ConvertTo-Json"'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts` around lines
2785 - 2792, The PowerShell snippet assigned to the variable "command" currently
replaces the process-level Path with only Machine+User registry values, dropping
VS Code-injected entries; update that snippet to preserve the current process
PATH by capturing it (e.g.,
$p=[Environment]::GetEnvironmentVariable('Path','Process') or using $e['Path'])
and append it when building $e['Path'] (set $e['Path']=$mp+';'+$up+';'+$p while
handling empty mp/up/p cases) so when updateProcessEnv writes process.env.Path
the VS Code entries remain and registry paths are prepended.
|
Duplicate of #1568 |
5 participants
Purpose
Update the
release/bi-1.8.xbranch with the Windows activation hotfix.Changes
Summary by CodeRabbit
Bug Fixes
Chores