fix(server): pick up newly installed provider CLIs on Windows refresh - #6356
fix(server): pick up newly installed provider CLIs on Windows refresh#6356fraineralex wants to merge 1 commit into
Conversation
Refresh was probing with a stale process PATH, so CLIs installed after launch stayed Not found until restart. Re-read User/Machine PATH (and known Codex dir) before re-probe. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.github.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟠 High
When loadProfile is true, the PATH captured by captureWindowsEnvironmentCommand ignores any PATH modifications made by the loaded PowerShell profile, because it only reads the Machine and User registry targets via [Environment]::GetEnvironmentVariable('PATH', 'Machine') and 'User' instead of the live process environment. A version manager that prepends its active Node/CLI directory to PATH during profile initialization will have that addition silently discarded, so installWindowsEnvironment cannot discover tools supplied only through profile logic. Consider falling back to the process PATH ($env:PATH) — or merging it with the Machine/User values — when a profile is loaded, so profile-injected entries are preserved.
Also found in 1 other location(s)
packages/shared/src/shell.ts:272
For
PATH,buildWindowsEnvironmentCaptureCommandignores the PowerShell process environment and always reconstructs the value from Machine and User targets. Consequently the fallback probe withloadProfile: truecannot return PATH additions made by the loaded profile; when Node or a provider CLI is supplied by profile initialization/version-manager logic rather than registry PATH,resolveWindowsEnvironmentstill reports it unavailable.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/shell/DesktopShellEnvironment.ts around line 246:
When `loadProfile` is `true`, the `PATH` captured by `captureWindowsEnvironmentCommand` ignores any PATH modifications made by the loaded PowerShell profile, because it only reads the `Machine` and `User` registry targets via `[Environment]::GetEnvironmentVariable('PATH', 'Machine')` and `'User'` instead of the live process environment. A version manager that prepends its active Node/CLI directory to PATH during profile initialization will have that addition silently discarded, so `installWindowsEnvironment` cannot discover tools supplied only through profile logic. Consider falling back to the process PATH (`$env:PATH`) — or merging it with the Machine/User values — when a profile is loaded, so profile-injected entries are preserved.
Also found in 1 other location(s):
- packages/shared/src/shell.ts:272 -- For `PATH`, `buildWindowsEnvironmentCaptureCommand` ignores the PowerShell process environment and always reconstructs the value from Machine and User targets. Consequently the fallback probe with `loadProfile: true` cannot return PATH additions made by the loaded profile; when Node or a provider CLI is supplied by profile initialization/version-manager logic rather than registry PATH, `resolveWindowsEnvironment` still reports it unavailable.
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d00be9516f
ℹ️ 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".
| "$machine = [Environment]::GetEnvironmentVariable('PATH', 'Machine')", | ||
| "$user = [Environment]::GetEnvironmentVariable('PATH', 'User')", | ||
| "$parts = @()", | ||
| "if ($null -ne $machine -and $machine.Length -gt 0) { $parts += $machine }", | ||
| "if ($null -ne $user -and $user.Length -gt 0) { $parts += $user }", | ||
| "$value = $parts -join ';'", |
There was a problem hiding this comment.
When loadProfile: true is used for the fnm/nvm fallback, the PowerShell profile updates the current process's $env:PATH, but these target-qualified reads discard it and return only the persistent Machine/User values. Consequently a Node or provider CLI made available by the profile still cannot be found after refresh; include the process PATH in the merge and mirror the fix in DesktopShellEnvironment.ts.
Useful? React with 👍 / 👎.
| const refreshInstance = Effect.fn("refreshInstance")(function* ( | ||
| instanceId: ProviderInstanceId, | ||
| ) { | ||
| yield* fixPath(); |
There was a problem hiding this comment.
When an instance defines even one environment override, every provider driver captures a cloned processEnv during construction via mergeProviderInstanceEnvironment; this call only updates HostProcessEnvironment/process.env, so providerSource.refresh continues probing with its stale cloned PATH. Newly installed CLIs therefore remain unavailable for configured instances until an instance rebuild or server restart; refresh the captured instance environment while preserving its overrides.
Useful? React with 👍 / 👎.
| const refresh = Effect.fn("refresh")(function* (provider?: ProviderDriverKind) { | ||
| yield* fixPath(); |
There was a problem hiding this comment.
On macOS and Linux, every provider refresh now calls fixPath(), whose POSIX path synchronously launches login shells with five-second timeouts before the provider probe begins. A slow or broken shell can therefore block the server event loop for several seconds whenever the WebSocket refresh action runs, affecting remote clients even though this change is intended only to discover Windows installations; guard this refresh-time repair to Windows.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
One finding: fixPath() is called inside ProviderRegistry service operations without providing the FileSystem/Path services the layer already acquired, so its requirements leak out of the layer into methods whose declared type (ProviderRegistryShape.refresh/refreshInstance) has no requirements. Also note this changes server behavior (every refresh now re-hydrates the process environment) without a focused apps/server test; consider extending ProviderRegistry.test.ts to cover it.
Posted via Macroscope — Effect Service Conventions
| }); | ||
|
|
||
| const refresh = Effect.fn("refresh")(function* (provider?: ProviderDriverKind) { | ||
| yield* fixPath(); |
There was a problem hiding this comment.
fixPath() requires FileSystem | Path, so this leaves those requirements in refresh's environment even though ProviderRegistryShape.refresh is declared as Effect.Effect<ReadonlyArray<ServerProvider>> (no requirements). The layer already acquires fileSystem/path at construction — consider providing them here as persistProvider does, and applying the same change to the refreshInstance call on line 493.
| yield* fixPath(); | |
| yield* fixPath().pipe( | |
| Effect.provideService(FileSystem.FileSystem, fileSystem), | |
| Effect.provideService(Path.Path, path), | |
| ); |
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.github.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d00be95. Configure here.
| "if ($null -ne $user -and $user.Length -gt 0) { $parts += $user }", | ||
| "$value = $parts -join ';'", | ||
| ] | ||
| : [`$value = [Environment]::GetEnvironmentVariable('${name}')`]; |
There was a problem hiding this comment.
Profile PATH ignored on Windows
High Severity
The Windows PATH probe always reads Machine and User registry values, including when loadProfile is enabled. Profile scripts mutate process PATH (for example fnm), so the profiled probe no longer surfaces those entries and the Windows profile fallback cannot discover profile-only directories.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d00be95. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Multiple unresolved High-severity findings identify that the new PATH capture logic ignores profile-modified PATH entries when loadProfile is true, and that fixPath() runs on all platforms rather than just Windows. These concerns affect the core functionality this PR aims to fix. You can customize Macroscope's approvability policy. Learn more. |
CDVolvik
left a comment
CDVolvik
left a comment
There was a problem hiding this comment.
Windows box check on d00be9516.
packages/shared/src/shell.test.ts + DesktopShellEnvironment.test.ts: 38/38 on the PR. Checking out origin/main over those two source files (keeping the PR tests) fails 5/38 — the new Machine/User command assertion and the Codex bin path. Those tests pin what they claim.
The stated bug is real. [Environment]::GetEnvironmentVariable('PATH') with no target is Process scope, so a User-PATH write after T3 started is invisible. Reading Machine+User from the registry is the right no-profile fix. On this machine that registry read is a superset of process PATH except five dirs (nodejs, Volta, pnpm, .bun, scoop) — exactly knownWindowsCliDirs, which you still prepend.
The problem is the same command is used for loadProfile: true. After this change the profile probe and the no-profile probe emit the same PATH source. $env:PATH mutations from a profile (fnm prepending FNM_MULTISHELL_PATH) cannot be observed. FNM_DIR / FNM_MULTISHELL_PATH are still captured from process scope, but PATH no longer includes the shim the profile prepends. Desktop still merges profile.PATH first, but it is the same string as noProfile.PATH.
The new tests mock execFile; they do not pin that the two probes differ. A mock that returns C:\Profile\Node still passes regardless of the command text.
Suggested split: keep Machine+User for -NoProfile, and keep process $env:PATH for loadProfile: true (the profile has already run). Programs\OpenAI\Codex\bin in knownWindowsCliDirs is fine.
fixPath() on every refresh/refreshInstance is already win32-gated inside, but on Linux/mac it now re-hydrates login-shell PATH on every provider refresh. No ProviderRegistry test covers that.
CDVolvik
left a comment
CDVolvik
left a comment
There was a problem hiding this comment.
Formalizing my earlier review as changes-requested — head is unchanged (d00be95) and the core issue stands:
Machine+User registry PATH is the right fix for the loadProfile: false probe, but the same command string is used for the loadProfile: true path — so profile-driven prepends (fnm's FNM_MULTISHELL_PATH shims, any $env:Path mutation in the profile) vanish from the one probe whose job is to capture them. That regresses exactly the users the profile probe exists for.
Scoping the registry-read command to the no-profile probe only (or taking the #4896 shape, which joins Process+Machine+User) resolves it. Detail in my earlier review on this commit.
2 participants
Fixes #6352
On Windows, if T3 starts without a provider CLI and you install it mid-session, Refresh still shows Not found until restart. The process PATH was frozen at launch, and the Windows env probe read process PATH instead of User/Machine.
fixPath()before provider refresh%LOCALAPPDATA%\Programs\OpenAI\Codex\binin known Windows CLI dirsDemo
Cold start without Codex → red → install → Refresh → green (no restart).
T3.Code.Fixed.mp4
Model: Grok 4.5