Enhance Windows environment detection in BallerinaExtension#1547
Enhance Windows environment detection in BallerinaExtension#1547axewilledge wants to merge 4 commits intowso2:release/bi-1.8.xfrom
Conversation
axewilledge
changed the title
📝 WalkthroughWalkthroughAdds a Windows fallback that locates the Ballerina binary by scanning Machine/User registry PATH entries and common installation directories. Integrates the discovered bin path into version detection and merges registry PATH entries into getShellEnvironment on Windows. Changes
Sequence DiagramsequenceDiagram
participant VSCode as VS Code Extension
participant VerDet as Version Detection
participant WinFB as Windows Fallback
participant Reg as Windows Registry
participant FS as File System
VSCode->>VerDet: Detect Ballerina version
rect rgba(200, 150, 100, 0.5)
Note over VerDet: ballerinaHome not provided
end
VerDet->>WinFB: findWindowsBallerinaPath()
WinFB->>Reg: Read registry PATH (Machine + User)
Reg-->>WinFB: Registry PATH values
WinFB->>FS: Search registry PATH for bal.bat
FS-->>WinFB: bal.bat found / not found
alt bal.bat found
WinFB-->>VerDet: Return bin directory
else not found
WinFB->>FS: Check common install directories (LOCALAPPDATA, ProgramFiles, versioned layouts)
FS-->>WinFB: bal.bat found / not found
WinFB-->>VerDet: Return bin directory or null
end
VerDet->>VerDet: Use discovered path as distPath and build shell env (merged PATH)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Pull request overview
This PR enhances Windows environment detection for the Ballerina extension by adding fallback mechanisms to locate the Ballerina installation when it's not found in the process PATH. This addresses scenarios where VS Code is launched before Ballerina installation or in restricted corporate environments with locked system PATH.
Changes:
- Added registry-based PATH retrieval to merge Machine and User scope environment variables on Windows
- Implemented fallback search strategy to locate Ballerina bin directory in common installation locations
- Enhanced shell environment retrieval to read from Windows registry instead of just process environment
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 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 2727-2733: The registry PATH entries may contain surrounding
double quotes so fs.existsSync(candidate) fails; in the loop that processes
rawPaths -> pathEntries (variables: rawPaths, pathEntries, entry, candidate)
normalize each entry by stripping surrounding quotes and trimming whitespace
before constructing candidate and before returning; ensure the returned value
uses the unquoted normalized entry (so debug, path.join and return use the
cleaned entry) and keep using fs.existsSync to check for 'bal.bat'.
- Around line 2786-2798: The current PowerShell command assigned to the variable
`command` in extension.ts replaces the process PATH with Machine/User registry
values and drops any session-specific PATH entries; update the PowerShell
snippet so it merges the registry PATH(s) with the existing Process PATH (i.e.,
read $e=[Environment]::GetEnvironmentVariables('Process'); get $mp and $up as
before, then set $e['Path'] = ($e['Path'] + ';' + $mp + ';' + $up) but only
include each segment if non-empty and avoid duplicate semicolons), ensuring the
final `$e | ConvertTo-Json` preserves session-injected PATH entries while still
including Machine and User registry values when present.
- Around line 1801-1812: The fallback sets distPath from
findWindowsBallerinaPath() which can be an unquoted Windows path with spaces,
causing cmd.exe to treat "C:\Program" as the executable; update the code that
constructs and runs the bal/bal.bat command (the code that uses distPath to form
the executable, e.g., the ballerinaCommand/bal invocation) to quote the
executable path when distPath is non-empty or, preferably, call the binary via
execFile with the executable path and args as separate parameters; ensure you
reference distPath and findWindowsBallerinaPath() when applying the change so
the resolved C:\Program Files\... path is correctly handled.
…g specific errors encountered during directory access.
…endencies and devDependencies.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
workspaces/ballerina/ballerina-extension/src/core/extension.ts (4)
2727-2733:⚠️ Potential issue | 🟡 MinorRegistry
PATHentries that include surrounding quotes are not stripped, causingfs.existsSyncto missbal.bat.Windows registry PATH entries frequently look like
"C:\Program Files\..."(with surrounding double-quotes).p.trim()only removes whitespace, sopath.join(entry, 'bal.bat')produces an invalid path and the entire Strategy 1 search silently fails for default installations.🤖 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 2727 - 2733, The PATH parsing currently trims whitespace but not surrounding quotes, so entries like "\"C:\\Program Files\\...\"" cause path.join(entry, 'bal.bat') to produce an invalid path and fs.existsSync to miss bal.bat; update the pathEntries creation to strip surrounding double-quotes from each entry (e.g. transform rawPaths.split(';').map(p => p.trim().replace(/^"+|"+$/g, '')...).filter(Boolean)) so the subsequent loop (variable entry, candidate = path.join(entry, 'bal.bat'), fs.existsSync(candidate)) operates on unquoted paths.
2787-2799:⚠️ Potential issue | 🟡 MinorProcess-scope PATH entries are dropped when registry values are present.
$e = [Environment]::GetEnvironmentVariables('Process')loads the current process PATH into$e['Path'], but the subsequentif/elseifblock immediately overwrites it with only$mp + ';' + $up, discarding any session-injected PATH (e.g., developer command prompts, VS Code's own additions). This can causebalresolution to regress in environments that previously worked.🤖 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 2787 - 2799, The PowerShell snippet assigned to the variable "command" currently overwrites the Process-scope PATH with only Machine/User registry values, discarding session-injected entries; change the merge so the Process PATH ($e['Path'] or $proc) is preserved and combined with the Machine ($mp) and User ($up) values (e.g., concatenate proc + mp + up in the desired precedence, removing empty segments and de-duplicating entries) before assigning back to $e['Path'], ensuring the rest of the script (ConvertTo-Json) remains unchanged.
1801-1812:⚠️ Potential issue | 🟠 MajorUnquoted fallback path with spaces will break
ballerinaCommandconstruction.When
findWindowsBallerinaPath()resolves to a path likeC:\Program Files\Ballerina\bin\, the command assembled at line 1834 —
C:\Program Files\Ballerina\bin\bal.bat version— causesexec/cmd.exeto treatC:\Programas the executable, making the fallback silently fail for default Windows installs.🤖 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 1801 - 1812, The Windows fallback may produce an unquoted command like C:\Program Files\Ballerina\bin\bal.bat version which breaks exec/cmd.exe; update the code that builds the Windows fallback command (the logic that uses distPath / findWindowsBallerinaPath() to call bal.bat) to quote or properly escape the full executable path when assembling ballerinaCommand (e.g., wrap the resolved path to bal.bat in double quotes or use a spawn/exec variant that accepts argv) so "C:\Program Files\Ballerina\bin\bal.bat" version is executed as a single executable token.
2720-2723:⚠️ Potential issue | 🟠 Major
execSyncinsidefindWindowsBallerinaPathblocks the extension-host event loop on every Windows activation.
findWindowsBallerinaPath()is called synchronously (line 1808) fromgetBallerinaVersionon every Windows startup wheneverballerinaHomeis not configured (the common case).execSyncwith a 10-second timeout can freeze VS Code's extension host for several seconds — or indefinitely when an antivirus scanner intercepts the PowerShell spawn.There is also a redundancy:
syncEnvironment()at line 1705 already launches an async PowerShell process that reads the sameMachine+Userregistry PATH.findWindowsBallerinaPathspawns a second PowerShell process unconditionally even when Ballerina is already resolvable via the PATH updated bysyncEnvironment. At minimum, guard the call with a quick check to skip the search whenbal.batis already reachable on the currentprocess.env.Path, and make the function async to avoid blocking:♻️ Suggested refactor — make async and add early-exit guard
- } else if (isWindows() && !ballerinaHome) { - const detectedBinPath = findWindowsBallerinaPath(); - if (detectedBinPath) { - distPath = detectedBinPath; - debug(`[VERSION] Windows fallback search found Ballerina bin: ${distPath}`); - } + } else if (isWindows() && !ballerinaHome) { + // Only run the expensive registry search when bal.bat is not already on PATH. + const alreadyOnPath = (process.env.Path || process.env.PATH || '') + .split(';').some(p => fs.existsSync(path.join(p.trim(), 'bal.bat'))); + if (!alreadyOnPath) { + const detectedBinPath = await findWindowsBallerinaPath(); + if (detectedBinPath) { + distPath = detectedBinPath; + debug(`[VERSION] Windows fallback search found Ballerina bin: ${distPath}`); + } + } }And change the function signature to use
execinstead ofexecSync:-function findWindowsBallerinaPath(): string { +async function findWindowsBallerinaPath(): Promise<string> { ... - const rawPaths = execSync( - `powershell.exe -NoProfile -Command "${psCommand}"`, - { encoding: 'utf8', timeout: 10000 } - ).trim(); + const rawPaths = await new Promise<string>((res, rej) => + exec(`powershell.exe -NoProfile -Command "${psCommand}"`, + { encoding: 'utf8', timeout: 10000 }, + (err, stdout) => err ? rej(err) : res(stdout.trim())) + );🤖 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 2720 - 2723, findWindowsBallerinaPath currently uses execSync which blocks the extension host; make it async (return a Promise) and switch from execSync to exec (or spawn) so the PowerShell call is non-blocking, and add an early-exit guard that checks whether "bal.bat" is already resolvable on process.env.Path before invoking any PowerShell; also update callers (e.g., getBallerinaVersion) to await the new async findWindowsBallerinaPath and avoid duplicating work when syncEnvironment already refreshed PATH by reusing that result.
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-extension/src/core/extension.ts (1)
2759-2771: Versioned subdirectory search uses filesystem order — oldest version may be selected.
fs.readdirSync(root)on NTFS returns entries in case-insensitive alphabetical order, soballerina-2201.11.0would be found and returned beforeballerina-2201.12.0, silently pinning users on an older installed distribution whenever multiple versions coexist.♻️ Suggested fix — sort descending before iterating
- const children = fs.readdirSync(root); + const children = fs.readdirSync(root).sort().reverse(); // newest version first 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 2759 - 2771, The directory iteration over children from fs.readdirSync(root) can pick an older version first; before the for-loop in the try block, sort the children array in descending version order (prefer highest/newest first) — e.g., replace iterating over children with children.sort((a,b) => b.localeCompare(a, undefined, {numeric: true, sensitivity: 'base'})) or a semver-aware comparator, then iterate; update references to children, root, versionedBin, and keep the same debug/error handling.common/config/rush/.pnpmfile.cjs (1)
64-66: LGTM —bn.jsoverride follows the established pattern correctly.The new entries are consistent with the surrounding override style and are not duplicated within the same block.
One unrelated, pre-existing maintenance nit:
diff,eslint, andlodashare each listed twice in both thedependenciesanddevDependenciessections (lines 37/52, 40/55, 46/61 etc.). Those redundant checks are harmless today but can silently mask a later version conflict if one copy is updated and the other is not.Also applies to: 115-117
🤖 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 64 - 66, There are redundant checks for the same packages in both pkg.dependencies and pkg.devDependencies (specifically the keys 'diff', 'eslint', and 'lodash'); remove the duplicate checks so each package override is performed only once per package map by consolidating logic to update either pkg.dependencies or pkg.devDependencies as appropriate, using the existing patterns around pkg.dependencies['bn.js'] and the pkg.devDependencies accessors to locate and dedupe those keys ('diff', 'eslint', 'lodash') so a single authoritative override is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2727-2733: The PATH parsing currently trims whitespace but not
surrounding quotes, so entries like "\"C:\\Program Files\\...\"" cause
path.join(entry, 'bal.bat') to produce an invalid path and fs.existsSync to miss
bal.bat; update the pathEntries creation to strip surrounding double-quotes from
each entry (e.g. transform rawPaths.split(';').map(p =>
p.trim().replace(/^"+|"+$/g, '')...).filter(Boolean)) so the subsequent loop
(variable entry, candidate = path.join(entry, 'bal.bat'),
fs.existsSync(candidate)) operates on unquoted paths.
- Around line 2787-2799: The PowerShell snippet assigned to the variable
"command" currently overwrites the Process-scope PATH with only Machine/User
registry values, discarding session-injected entries; change the merge so the
Process PATH ($e['Path'] or $proc) is preserved and combined with the Machine
($mp) and User ($up) values (e.g., concatenate proc + mp + up in the desired
precedence, removing empty segments and de-duplicating entries) before assigning
back to $e['Path'], ensuring the rest of the script (ConvertTo-Json) remains
unchanged.
- Around line 1801-1812: The Windows fallback may produce an unquoted command
like C:\Program Files\Ballerina\bin\bal.bat version which breaks exec/cmd.exe;
update the code that builds the Windows fallback command (the logic that uses
distPath / findWindowsBallerinaPath() to call bal.bat) to quote or properly
escape the full executable path when assembling ballerinaCommand (e.g., wrap the
resolved path to bal.bat in double quotes or use a spawn/exec variant that
accepts argv) so "C:\Program Files\Ballerina\bin\bal.bat" version is executed as
a single executable token.
- Around line 2720-2723: findWindowsBallerinaPath currently uses execSync which
blocks the extension host; make it async (return a Promise) and switch from
execSync to exec (or spawn) so the PowerShell call is non-blocking, and add an
early-exit guard that checks whether "bal.bat" is already resolvable on
process.env.Path before invoking any PowerShell; also update callers (e.g.,
getBallerinaVersion) to await the new async findWindowsBallerinaPath and avoid
duplicating work when syncEnvironment already refreshed PATH by reusing that
result.
---
Nitpick comments:
In `@common/config/rush/.pnpmfile.cjs`:
- Around line 64-66: There are redundant checks for the same packages in both
pkg.dependencies and pkg.devDependencies (specifically the keys 'diff',
'eslint', and 'lodash'); remove the duplicate checks so each package override is
performed only once per package map by consolidating logic to update either
pkg.dependencies or pkg.devDependencies as appropriate, using the existing
patterns around pkg.dependencies['bn.js'] and the pkg.devDependencies accessors
to locate and dedupe those keys ('diff', 'eslint', 'lodash') so a single
authoritative override is applied.
In `@workspaces/ballerina/ballerina-extension/src/core/extension.ts`:
- Around line 2759-2771: The directory iteration over children from
fs.readdirSync(root) can pick an older version first; before the for-loop in the
try block, sort the children array in descending version order (prefer
highest/newest first) — e.g., replace iterating over children with
children.sort((a,b) => b.localeCompare(a, undefined, {numeric: true,
sensitivity: 'base'})) or a semver-aware comparator, then iterate; update
references to children, root, versionedBin, and keep the same debug/error
handling.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/config/rush/.pnpmfile.cjsworkspaces/ballerina/ballerina-extension/src/core/extension.ts
2 participants
Purpose
Goals
Approach
Added a fallback mechanism to locate the Ballerina bin directory using registry paths and common installation directories when the PATH is not updated. This improves usability on restricted environments.
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