fix(path): add Windows PATH bootstrap dirs#44215
fix(path): add Windows PATH bootstrap dirs#44215aniruddhaadak80 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR extends the PATH bootstrap helper (
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/path-env.test.ts
Line: 250-255
Comment:
**`ProgramFiles(x86)` branch is not exercised**
The test sets `ProgramFiles` and `ProgramW6432` but never sets `ProgramFiles(x86)`. As a result, the `programFilesX86` branch in `path-env.ts` (line 115–117) has zero test coverage and `programFilesX86/nodejs` is never verified in the assertion. Consider adding the env-var and including its resolved dir in the `arrayContaining` check.
```suggestion
process.env.PATH = nodejsDir;
process.env.LOCALAPPDATA = localAppData;
process.env.APPDATA = appData;
process.env.ProgramFiles = programFiles;
process.env.ProgramW6432 = programFiles;
process.env["ProgramFiles(x86)"] = abs("C:/Program Files (x86)");
process.env.SystemRoot = systemRoot;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/path-env.test.ts
Line: 256
Comment:
**HOMEBREW / XDG env vars not cleared before Windows test**
Every other test that cares about PATH ordering explicitly clears `HOMEBREW_PREFIX`, `HOMEBREW_BREW_FILE`, and `XDG_BIN_HOME` (see the Linuxbrew test at lines 206–208). This test skips those deletes. On a macOS/Linux CI runner where those variables are set, `resolveBrewPathDirs` could inject additional entries into `prepend`, making the actual PATH content non-deterministic and potentially masking ordering regressions. Since the assertion uses `arrayContaining`, failures won't surface today, but the test environment is not fully isolated.
Consider adding:
```suggestion
delete process.env.OPENCLAW_PATH_BOOTSTRAPPED;
delete process.env.HOMEBREW_PREFIX;
delete process.env.HOMEBREW_BREW_FILE;
delete process.env.XDG_BIN_HOME;
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 696dc61 |
| process.env.PATH = nodejsDir; | ||
| process.env.LOCALAPPDATA = localAppData; | ||
| process.env.APPDATA = appData; | ||
| process.env.ProgramFiles = programFiles; | ||
| process.env.ProgramW6432 = programFiles; | ||
| process.env.SystemRoot = systemRoot; |
There was a problem hiding this comment.
ProgramFiles(x86) branch is not exercised
The test sets ProgramFiles and ProgramW6432 but never sets ProgramFiles(x86). As a result, the programFilesX86 branch in path-env.ts (line 115–117) has zero test coverage and programFilesX86/nodejs is never verified in the assertion. Consider adding the env-var and including its resolved dir in the arrayContaining check.
| process.env.PATH = nodejsDir; | |
| process.env.LOCALAPPDATA = localAppData; | |
| process.env.APPDATA = appData; | |
| process.env.ProgramFiles = programFiles; | |
| process.env.ProgramW6432 = programFiles; | |
| process.env.SystemRoot = systemRoot; | |
| process.env.PATH = nodejsDir; | |
| process.env.LOCALAPPDATA = localAppData; | |
| process.env.APPDATA = appData; | |
| process.env.ProgramFiles = programFiles; | |
| process.env.ProgramW6432 = programFiles; | |
| process.env["ProgramFiles(x86)"] = abs("C:/Program Files (x86)"); | |
| process.env.SystemRoot = systemRoot; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/path-env.test.ts
Line: 250-255
Comment:
**`ProgramFiles(x86)` branch is not exercised**
The test sets `ProgramFiles` and `ProgramW6432` but never sets `ProgramFiles(x86)`. As a result, the `programFilesX86` branch in `path-env.ts` (line 115–117) has zero test coverage and `programFilesX86/nodejs` is never verified in the assertion. Consider adding the env-var and including its resolved dir in the `arrayContaining` check.
```suggestion
process.env.PATH = nodejsDir;
process.env.LOCALAPPDATA = localAppData;
process.env.APPDATA = appData;
process.env.ProgramFiles = programFiles;
process.env.ProgramW6432 = programFiles;
process.env["ProgramFiles(x86)"] = abs("C:/Program Files (x86)");
process.env.SystemRoot = systemRoot;
```
How can I resolve this? If you propose a fix, please make it concise.| process.env.ProgramFiles = programFiles; | ||
| process.env.ProgramW6432 = programFiles; | ||
| process.env.SystemRoot = systemRoot; | ||
| delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; |
There was a problem hiding this comment.
HOMEBREW / XDG env vars not cleared before Windows test
Every other test that cares about PATH ordering explicitly clears HOMEBREW_PREFIX, HOMEBREW_BREW_FILE, and XDG_BIN_HOME (see the Linuxbrew test at lines 206–208). This test skips those deletes. On a macOS/Linux CI runner where those variables are set, resolveBrewPathDirs could inject additional entries into prepend, making the actual PATH content non-deterministic and potentially masking ordering regressions. Since the assertion uses arrayContaining, failures won't surface today, but the test environment is not fully isolated.
Consider adding:
| delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; | |
| delete process.env.OPENCLAW_PATH_BOOTSTRAPPED; | |
| delete process.env.HOMEBREW_PREFIX; | |
| delete process.env.HOMEBREW_BREW_FILE; | |
| delete process.env.XDG_BIN_HOME; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/path-env.test.ts
Line: 256
Comment:
**HOMEBREW / XDG env vars not cleared before Windows test**
Every other test that cares about PATH ordering explicitly clears `HOMEBREW_PREFIX`, `HOMEBREW_BREW_FILE`, and `XDG_BIN_HOME` (see the Linuxbrew test at lines 206–208). This test skips those deletes. On a macOS/Linux CI runner where those variables are set, `resolveBrewPathDirs` could inject additional entries into `prepend`, making the actual PATH content non-deterministic and potentially masking ordering regressions. Since the assertion uses `arrayContaining`, failures won't surface today, but the test environment is not fully isolated.
Consider adding:
```suggestion
delete process.env.OPENCLAW_PATH_BOOTSTRAPPED;
delete process.env.HOMEBREW_PREFIX;
delete process.env.HOMEBREW_BREW_FILE;
delete process.env.XDG_BIN_HOME;
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 696dc61097
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prepend.push( | ||
| path.join(localAppData, "pnpm"), | ||
| path.join(localAppData, "Microsoft", "WindowsApps"), | ||
| ); |
There was a problem hiding this comment.
Avoid prepending WindowsApps ahead of existing PATH
Adding %LOCALAPPDATA%\\Microsoft\\WindowsApps to the prepend list can change command resolution in non-minimal Windows setups, because mergePath places all prepend entries before the existing PATH. On machines where Windows App Execution Aliases are enabled (for example python.exe in WindowsApps) and a real interpreter already exists later in PATH, this change causes OpenClaw subprocesses to resolve the alias/stub instead of the intended executable, which can break tool execution unexpectedly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR extends PATH bootstrapping in ensureOpenClawCliOnPath to better support Windows environments by adding common Windows tool locations to the candidate directories, and adds a Vitest case to validate the new behavior.
Changes:
- Add Windows-specific PATH candidate directories (LOCALAPPDATA pnpm/WindowsApps, APPDATA npm, Program Files nodejs, SystemRoot/System32).
- Extend the PATH-bootstrapping test suite to cover Windows tool directory prepending.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/infra/path-env.ts | Adds Windows-specific candidate PATH entries to the bootstrap logic. |
| src/infra/path-env.test.ts | Adds a Windows-focused test and snapshots additional Windows env vars. |
| expect(parts).toEqual( | ||
| expect.arrayContaining([pnpmDir, windowsAppsDir, npmDir, nodejsDir, system32Dir, systemRoot]), | ||
| ); | ||
| expect(parts.indexOf(pnpmDir)).toBeLessThan(parts.indexOf(nodejsDir)); |
There was a problem hiding this comment.
This test claims to verify that common Windows tool dirs are prepended, but it only asserts ordering for pnpmDir vs nodejsDir. Because arrayContaining ignores order, the test would still pass if some of the other Windows dirs (e.g. windowsAppsDir, npmDir, system32Dir, systemRoot) were appended after nodejsDir. Consider asserting that each of these expected directories appears before the original PATH entry (and optionally that System32 comes before SystemRoot).
| expect(parts.indexOf(pnpmDir)).toBeLessThan(parts.indexOf(nodejsDir)); | |
| const originalPathIndex = parts.indexOf(nodejsDir); | |
| expect(originalPathIndex).toBeGreaterThanOrEqual(0); | |
| const pnpmIndex = parts.indexOf(pnpmDir); | |
| const windowsAppsIndex = parts.indexOf(windowsAppsDir); | |
| const npmIndex = parts.indexOf(npmDir); | |
| const system32Index = parts.indexOf(system32Dir); | |
| const systemRootIndex = parts.indexOf(systemRoot); | |
| expect(pnpmIndex).toBeGreaterThanOrEqual(0); | |
| expect(windowsAppsIndex).toBeGreaterThanOrEqual(0); | |
| expect(npmIndex).toBeGreaterThanOrEqual(0); | |
| expect(system32Index).toBeGreaterThanOrEqual(0); | |
| expect(systemRootIndex).toBeGreaterThanOrEqual(0); | |
| // All common tool dirs should be prepended before the original PATH entry. | |
| expect(pnpmIndex).toBeLessThan(originalPathIndex); | |
| expect(windowsAppsIndex).toBeLessThan(originalPathIndex); | |
| expect(npmIndex).toBeLessThan(originalPathIndex); | |
| expect(system32Index).toBeLessThan(originalPathIndex); | |
| expect(systemRootIndex).toBeLessThan(originalPathIndex); | |
| // Optionally, ensure System32 comes before SystemRoot. | |
| expect(system32Index).toBeLessThan(systemRootIndex); |
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09fb203251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| platform: "win32", | ||
| }); | ||
|
|
||
| const parts = (process.env.PATH ?? "").split(path.delimiter); |
There was a problem hiding this comment.
Parse simulated Windows PATH with semicolon delimiter
This test case forces platform: "win32", and ensureOpenClawCliOnPath now merges PATH entries with ";" on that platform (src/infra/path-env.ts:151), but the assertion still splits using path.delimiter. In the Ubuntu checks lane (.github/workflows/ci.yml), path.delimiter is ":", so Windows-style entries like C:/... get split at the drive colon and the expected directories cannot be found, making this regression test fail on non-Windows CI.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: found issues before merge. Summary Reproducibility: yes. Source inspection shows current main still has no win32-specific bootstrap branch, and the PR test failure is source-reproducible because it splits a semicolon-joined simulated Windows PATH with the host delimiter. Next step before merge Security Review findings
Review detailsBest possible solution: Revise or replace the PR so Windows bootstrap follows the existing PATH hardening policy: trusted runtime/system roots may be searched ahead of PATH, user-profile package-manager shims should be appended or explicitly opted in, tests should use deterministic Windows delimiters, and unrelated schema-label changes should be dropped. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main still has no win32-specific bootstrap branch, and the PR test failure is source-reproducible because it splits a semicolon-joined simulated Windows PATH with the host delimiter. Is this the best way to solve the issue? No. Windows bootstrap support is the right direction, but this diff is not the best fix because it elevates user-writable Windows shim directories ahead of trusted paths and mixes in stale unrelated labels. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a90be474f441. |
Summary
gh,node, or user-installed package manager shims.env.vars.PATHmerging logic, process spawning, or non-Windows path ordering.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
Repro + Verification
Environment
Steps
ensureOpenClawCliOnPath.Expected
Actual
Evidence
Human Verification (required)
pnpm exec vitest run src/infra/path-env.test.ts src/agents/bash-tools.test.ts -t "PATH|prepends common Windows tool dirs when present".pnpm build:plugin-sdk:dts.Compatibility / Migration
Failure Recovery (if this breaks)