fix(server): Provider updates always fail on Windows#2781
Conversation
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
size:M
Haplois
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR propagates provider-specific environment variables into maintenance update commands, improves Windows compatibility for spawning update processes, and normalizes PATH handling on Windows to avoid casing-related issues.
Changes:
- Add optional
envto provider maintenance update actions and pass it through to spawned child processes (withshell: trueon Windows). - Update capability resolution to carry
envthrough to the resolved update command. - Normalize merged environment PATH casing on Windows and expand test coverage around env propagation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/server/src/provider/providerMaintenanceRunner.ts | Passes per-provider env into the spawned update process and enables Windows shell execution. |
| apps/server/src/provider/providerMaintenanceRunner.test.ts | Extends spawner mock to capture spawn options and asserts env/shell propagation. |
| apps/server/src/provider/providerMaintenance.ts | Adds env to maintenance command action and threads env through capability resolution. |
| apps/server/src/provider/providerMaintenance.test.ts | Adds test ensuring provider env is attached to resolved update commands. |
| apps/server/src/provider/ProviderInstanceEnvironment.ts | Normalizes PATH casing on Windows when merging env overrides. |
| apps/server/src/provider/ProviderInstanceEnvironment.test.ts | Adds Windows PATH-casing normalization test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .spawn( | ||
| ChildProcess.make(input.command, [...input.args], { | ||
| ...(input.env ? { env: input.env } : {}), | ||
| ...(process.platform === "win32" ? { shell: true } : {}), | ||
| }), | ||
| ) |
There was a problem hiding this comment.
This is the most minimal fix for update command resolution on Windows. This lets Windows resolve shims like npm.cmd, bun.cmd, and other package-manager/provider launchers.
The injection risk is limited, because command and args are generated internally in by providerMaintanance.ts, not accepted directly from user input. That said, the concern is valid as a hardening issue: using a shell broadly expands parsing/quoting semantics and increases risk if future update capabilities become externally influenced.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review The fix adds You can customize Macroscope's approvability policy. Learn more. |
github-actions
Bot
added
size:XS
Haplois
force-pushed
the
main
branch
2 times, most recently
from
7d561b3 to
a08e55c
Compare
Haplois
force-pushed
the
main
branch
2 times, most recently
from
4da9c59 to
124c034
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5596281. Configure here.
| if (descriptor) { | ||
| Object.defineProperty(process, "platform", descriptor); | ||
| } | ||
| }), |
There was a problem hiding this comment.
Platform mock cannot restore
Low Severity
The new withProcessPlatform helper redefines process.platform with only { value: platform }, which makes the property non-configurable by default. Teardown then often cannot reapply the saved descriptor, so cleanup throws or process.platform stays win32 for later tests in the same worker.
Reviewed by Cursor Bugbot for commit 5596281. Configure here.
What Changed
Provider update commands now run with the provider instance’s resolved environment. On Windows, provider environments also canonicalize
PATHcasing and update commands run through the shell so.cmd/shim-based package manager commands resolve correctly.Tested on Windows 11 (a clean lab install + my local env) and Ubuntu 24.04.4 LTS. I don't have an OSX device to test.
Fixes #2765
Why
Windows can expose
PATHwith different casing, and provider-specific environments were used for detection but not passed through to the actual update command. That meant T3 Code could detect the right driver/update path but then fail to run the updater because the spawned process did not see the same environment or command resolution behavior.Checklist
No UI changes.
Note
Low Risk
Small, platform-gated spawn option change aligned with existing provider command patterns; non-Windows behavior unchanged.
Overview
Fixes provider one-click updates on Windows by spawning maintenance update commands (e.g.
npm install -g …) with{ shell: true }whenprocess.platform === 'win32', matching how other provider subprocess calls already behave on Windows so.cmd/shim executables resolve.Tests now pass
ChildProcess.CommandOptionsthrough the mock spawner, add awithProcessPlatformhelper, and assertshell: trueon simulatedwin32duringupdateProvider.Reviewed by Cursor Bugbot for commit 5596281. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
[!NOTE]
Fix provider update commands always failing on Windows by passing
shell: trueOn Windows,
npmis a batch script and cannot be spawned directly without a shell. providerMaintenanceRunner.ts now passes{ shell: true }asCommandOptionstoChildProcess.makewhenprocess.platform === 'win32', leaving other platforms unchanged. A new test uses awithProcessPlatformhelper to simulatewin32and asserts the shell option is set.Macroscope summarized 5596281.