Skip to content

fix(server): Provider updates always fail on Windows#2781

Open
Haplois wants to merge 1 commit into
pingdotgg:mainfrom
Haplois:main
Open

fix(server): Provider updates always fail on Windows#2781
Haplois wants to merge 1 commit into
pingdotgg:mainfrom
Haplois:main

Conversation

@Haplois
Copy link
Copy Markdown

@Haplois Haplois commented May 22, 2026

What Changed

Provider update commands now run with the provider instance’s resolved environment. On Windows, provider environments also canonicalize PATH casing 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 PATH with 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

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

No UI changes.


Note

Medium Risk
Changes provider update process spawning behavior on Windows by enabling shell: true, which could affect command execution and quoting; non-Windows platforms are unchanged.

Overview
Fixes provider updates on Windows by spawning update commands with ChildProcess.make(..., { shell: true }) when process.platform === "win32", allowing .cmd/shim executables like npm to resolve correctly.

Updates the test harness to capture ChildProcess.CommandOptions, adds a withProcessPlatform helper to simulate Windows, and asserts the update command is invoked with shell: true.

Reviewed by Cursor Bugbot for commit a08e55c. 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: true

On Windows, npm is a batch script and cannot be spawned directly without a shell. providerMaintenanceRunner.ts now passes { shell: true } as CommandOptions to ChildProcess.make when process.platform === 'win32', leaving other platforms unchanged. A new test uses a withProcessPlatform helper to simulate win32 and asserts the shell option is set.

Macroscope summarized a08e55c.

Copilot AI review requested due to automatic review settings May 22, 2026 06:12
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1b9ed5bc-fa2e-41ad-872e-3977b6781fc9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list. labels May 22, 2026
@Haplois Haplois changed the title fix(server): run provider updates with instance env on Windows fix(server): Provider updates always fail on Windows May 22, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 env to provider maintenance update actions and pass it through to spawned child processes (with shell: true on Windows).
  • Update capability resolution to carry env through 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.

Comment thread apps/server/src/provider/providerMaintenance.ts Outdated
Comment thread apps/server/src/provider/providerMaintenance.ts Outdated
Comment thread apps/server/src/provider/ProviderInstanceEnvironment.ts Outdated
Comment thread apps/server/src/provider/ProviderInstanceEnvironment.ts Outdated
Comment on lines +80 to +85
.spawn(
ChildProcess.make(input.command, [...input.args], {
...(input.env ? { env: input.env } : {}),
...(process.platform === "win32" ? { shell: true } : {}),
}),
)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread apps/server/src/provider/ProviderInstanceEnvironment.test.ts Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4c2ce06. Configure here.

Comment thread apps/server/src/provider/ProviderInstanceEnvironment.ts
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp Bot commented May 22, 2026

Approvability

Verdict: Needs human review

The fix adds shell: true for Windows process spawning, which has an unresolved security concern about potential command injection if command/args can be influenced by external config. Human review should verify the command inputs are properly constrained.

You can customize Macroscope's approvability policy. Learn more.

@github-actions github-actions Bot added size:XS 0-9 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Codex Update Always Failing

2 participants