[Hotfix] Enhance Windows environment detection in BallerinaExtension#1549
[Hotfix] Enhance Windows environment detection in BallerinaExtension#1549axewilledge wants to merge 5 commits intowso2:hotfix/windows-fixfrom
Conversation
…g specific errors encountered during directory access.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPNPM configuration updated to synchronize bn.js version to 5.2.3 across dependencies. Windows-specific fallback mechanism added to locate Ballerina binary via registry and common installation directories, with enhanced environment variable handling and debug logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
…endencies and devDependencies.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-extension/src/core/extension.ts (1)
2752-2760: Versioned subdirectory picks the first filesystem-order match — may resolve to a stale version.
readdirSyncorder is non-deterministic. If multiple Ballerina versions exist under a root (e.g.,ballerina-2201.8.0andballerina-2201.12.0), this could return the older one.Sort versioned directories in descending order to prefer the latest
try { - const children = fs.readdirSync(root); + const children = fs.readdirSync(root) + .filter(c => c.toLowerCase().startsWith('ballerina')) + .sort() + .reverse(); for (const child of children) {🤖 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 2752 - 2760, The code uses fs.readdirSync(root) and returns the first match, which can pick a stale Ballerina version; before iterating children, sort the array of directory names in descending (newest-first) order so the loop in extension.ts that builds versionedBin (using path.join and checking fs.existsSync(path.join(versionedBin, 'bal.bat'))) prefers the latest version; implement a sort on children (e.g., children.sort(...).reverse() or children.sort((a,b)=> b.localeCompare(a, undefined, {numeric:true, sensitivity:'base'}))) before the for..of to ensure newest version is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2780-2792: The PowerShell snippet assigned to command currently
overwrites $e['Path'] with registry-only values ($mp + $up), which drops VS
Code-injected entries when updateProcessEnv later sets process.env.Path; change
the logic in the command construction so that $e['Path'] preserves the existing
process Path ($e['Path']) and prepends or merges the registry Machine/User
values ($mp, $up) into it (avoid simple replacement), ensuring process.env.Path
retains VS Code/internal paths after updateProcessEnv uses the returned
newEnv.Path.
---
Nitpick comments:
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2752-2760: The code uses fs.readdirSync(root) and returns the
first match, which can pick a stale Ballerina version; before iterating
children, sort the array of directory names in descending (newest-first) order
so the loop in extension.ts that builds versionedBin (using path.join and
checking fs.existsSync(path.join(versionedBin, 'bal.bat'))) prefers the latest
version; implement a sort on children (e.g., children.sort(...).reverse() or
children.sort((a,b)=> b.localeCompare(a, undefined, {numeric:true,
sensitivity:'base'}))) before the for..of to ensure newest version is chosen.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/config/rush/.pnpmfile.cjsworkspaces/ballerina/ballerina-extension/src/core/extension.ts
| // Windows: read from registry (Machine + User scopes) so that paths added by | ||
| // a fresh Ballerina install (which goes to the User PATH registry key) are | ||
| // picked up even when VS Code's process was launched before the installation. | ||
| // We start with the current Process environment so that VS Code-internal | ||
| // variables are preserved, but we override Path with the merged registry value. | ||
| 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.
Overriding Path with registry-only values drops VS Code-internal PATH entries.
The new PowerShell command replaces $e['Path'] with Machine + User from the registry. Any paths that VS Code (or its extension host) injected into the process environment at runtime — for example, its embedded git or node binary directories — will be lost when updateProcessEnv later sets process.env.Path = newEnv.Path.
Consider merging the registry values with the current process PATH instead of replacing it, so that VS Code-internal tooling remains functional:
Proposed fix: merge registry PATH with existing process PATH
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};' +
+ '$pp=$e[\'Path\'];' +
+ '$merged=@($mp,$up,$pp|Where-Object{$_})-join\';\';' +
+ 'if($merged){$e[\'Path\']=$merged};' +
'$e | ConvertTo-Json"';This keeps existing process Path entries (including VS Code-injected ones) while prepending the registry Machine and User values so that freshly-installed tools are found first.
📝 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.
| // Windows: read from registry (Machine + User scopes) so that paths added by | |
| // a fresh Ballerina install (which goes to the User PATH registry key) are | |
| // picked up even when VS Code's process was launched before the installation. | |
| // We start with the current Process environment so that VS Code-internal | |
| // variables are preserved, but we override Path with the merged registry value. | |
| 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\';\';' + | |
| 'if($merged){$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
2780 - 2792, The PowerShell snippet assigned to command currently overwrites
$e['Path'] with registry-only values ($mp + $up), which drops VS Code-injected
entries when updateProcessEnv later sets process.env.Path; change the logic in
the command construction so that $e['Path'] preserves the existing process Path
($e['Path']) and prepends or merges the registry Machine/User values ($mp, $up)
into it (avoid simple replacement), ensuring process.env.Path retains VS
Code/internal paths after updateProcessEnv uses the returned newEnv.Path.
There was a problem hiding this comment.
Pull request overview
Improves Windows-specific Ballerina CLI discovery for the VS Code Ballerina extension, targeting cases where VS Code’s inherited PATH is stale or restricted (per linked BI issues).
Changes:
- Add a Windows fallback search that locates
bal.batvia merged registry PATH and common install directories. - Update Windows shell-environment retrieval to merge Machine+User PATH from the registry into the returned env.
- Pin
bn.jsto5.2.3via Rush/PNPM hooks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
workspaces/ballerina/ballerina-extension/src/core/extension.ts |
Adds Windows fallback bin detection and adjusts Windows environment PATH sourcing to use registry values. |
common/config/rush/.pnpmfile.cjs |
Adds a dependency/devDependency override to pin bn.js to a specific version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const detectedBinPath = findWindowsBallerinaPath(); | ||
| if (detectedBinPath) { | ||
| distPath = detectedBinPath; | ||
| debug(`[VERSION] Windows fallback search found Ballerina bin: ${distPath}`); | ||
| } |
There was a problem hiding this comment.
findWindowsBallerinaPath() can return an absolute bin path under locations like C:\Program Files\.... That distPath is later concatenated into a command string (e.g. distPath + 'bal' + exeExtension) and executed via exec() / cmd.exe /c. Without quoting or switching to execFile/spawn with args, paths containing spaces will fail to execute on Windows. Consider returning a fully-quoted executable path, or (preferably) avoid shell parsing by invoking bal.bat via spawn/execFile with an explicit file path + args.
| } else if (isWindows() && !ballerinaHome) { | ||
| // On Windows, if syncEnvironment() already merged the User+Machine PATH the | ||
| // 'bal.bat version' call below will just work via PATH lookup (distPath stays | ||
| // empty). But for restricted environments (where even User | ||
| // PATH is locked, or where VSCode's inherited PATH is still stale), we run a | ||
| // proactive directory search here so that we can use an absolute path instead | ||
| // of relying on PATH resolution. | ||
| const detectedBinPath = findWindowsBallerinaPath(); | ||
| if (detectedBinPath) { | ||
| distPath = detectedBinPath; | ||
| debug(`[VERSION] Windows fallback search found Ballerina bin: ${distPath}`); |
There was a problem hiding this comment.
This fallback search runs unconditionally whenever isWindows() && !ballerinaHome, even though the comment suggests PATH-based resolution will often already work after syncEnvironment(). Since findWindowsBallerinaPath() does synchronous PowerShell + filesystem scans (up to 10s timeout), consider only running it after the initial bal(.bat) version attempt fails (e.g. ENOENT / "not recognized"), and/or caching the detected result so repeated version checks don’t repeatedly invoke PowerShell.
| 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.
findWindowsBallerinaPath() returns entry + path.sep when it finds a PATH entry containing bal.bat. If the PATH entry already ends with a separator, this can produce a double separator (e.g. ...\\). It’s safer to normalize before returning (e.g. ensure exactly one trailing separator) so downstream string concatenations produce consistent command paths.
…y version 10.2.1 in pnpmfile.cjs.
3 participants
Purpose
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Bug Fixes
Chores