Fix Windows provider startup and update issues#2609
Fix Windows provider startup and update issues#2609riftzen-bit wants to merge 1 commit intopingdotgg:mainfrom
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple Windows-specific failures encountered when running the desktop app locally, primarily by improving how provider maintenance/update commands are spawned on Windows and by preventing unsupported Codex serviceTier values from being sent to codex app-server.
Changes:
- Resolve provider maintenance spawn commands on Windows to the concrete shim/executable path (e.g.
npm.cmd) before spawning. - Omit unsupported Codex
serviceTiervalues fromthread/startandturn/startpayloads. - Remove Windows
shell: trueusage from the server build CLI command (now usesprocess.execPath --run ...) and harden a set of Windows-sensitive tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/mock-update-server.test.ts | Skips symlink-escape assertion when symlink creation fails on Windows. |
| oxlint-plugin-t3code/test/utils.ts | Adds longer per-test timeout for oxlint rule harness tests. |
| apps/web/src/lib/archivedThreadsState.ts | Import reordering (no functional change). |
| apps/server/src/provider/providerMaintenanceRunner.ts | Adds Windows command-path resolution before spawning update processes. |
| apps/server/src/provider/providerMaintenanceRunner.test.ts | Adds/updates tests for Windows command shim resolution and resolved spawn commands. |
| apps/server/src/provider/Layers/CodexSessionRuntime.ts | Normalizes/filters serviceTier to supported values (fast/flex) before sending to Codex. |
| apps/server/src/provider/Layers/CodexSessionRuntime.test.ts | Adds coverage asserting unsupported tiers are omitted from Codex payloads. |
| apps/server/scripts/cli.ts | Removes shell: true workaround for Windows; uses process.execPath for build command. |
| apps/desktop/src/app/DesktopEnvironment.test.ts | Normalizes paths in assertions to be Windows-compatible. |
| apps/desktop/src/app/DesktopAppIdentity.test.ts | Normalizes Windows paths in assertions for user data directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const symlinkCreated = yield* fileSystem.symlink(outsideFile, symlinkPath).pipe( | ||
| Effect.as(true), | ||
| Effect.catch((error) => | ||
| process.platform === "win32" ? Effect.succeed(false) : Effect.fail(error), | ||
| ), | ||
| ); | ||
| if (!symlinkCreated) { | ||
| return; | ||
| } |
| it("resolves Windows command shims before spawning update commands", () => { | ||
| const resolvedNpm = ProviderMaintenanceRunner.resolveProviderMaintenanceSpawnCommand("npm", { | ||
| platform: "win32", | ||
| env: process.env, | ||
| }); | ||
| if (resolvedNpm !== "npm") { | ||
| assert.match(resolvedNpm, /npm\.(cmd|exe|bat|com)$/i); | ||
| } |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved Platform-specific bug fix for Windows with most changes in test files (path normalization, timeouts). Runtime changes are limited to Windows command resolution for spawning and filtering unsupported service tier values before API calls. Open review comments address test quality rather than production issues. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
this is fixed on main. was a breaking change in the app server contract where they started returning priority when the old shcema was a stricter union. now it's just string on the protocol.
depending on the codex version you're on you need to be on nightly or not of t3code though as the protocol change most likely hasn't gone out to latest yet
What changed
This fixes the Windows-specific provider failures I hit while running the desktop app locally:
npm install -g @openai/codex@latestrun through the actualnpm.cmdshim instead of failing withChildProcess.spawn NotFound.codex app-server; unsupported values are now omitted instead of causing thread/start or turn/start failures.Why
On Windows, the desktop app could start, but provider maintenance failed when the server tried to spawn
npmdirectly. Node process spawning does not reliably resolve.cmdshims from a bare command name, which is exactly how npm is installed on Windows. The update runner now resolves the command path first and keeps the existing non-Windows behavior unchanged.Validation
Ran after rebasing onto current
origin/main:bun fmtbun lint(0 errors; existing warnings remain)bun typecheckbun run buildbun --filter t3 test src/provider/providerMaintenanceRunner.test.ts src/provider/Layers/CodexSessionRuntime.test.tsbun --filter @t3tools/desktop test src/app/DesktopEnvironment.test.ts src/app/DesktopAppIdentity.test.tsbun --filter @t3tools/scripts test mock-update-server.test.tsbun --filter @t3tools/oxlint-plugin-t3code testNote
Fix Windows provider startup and update command resolution
resolveProviderMaintenanceSpawnCommandin providerMaintenanceRunner.ts to resolve Windows command shims (e.g.npm.cmd) before spawning maintenance processes instead of using bare command names.shell: trueworkaround from the build script's child process invocation on Windows in cli.ts.normalizeCodexServiceTierin CodexSessionRuntime.ts to drop unsupported service tiers (anything other than'fast'or'flex') from thread and turn start payloads.Macroscope summarized dba5fae.
Note
Medium Risk
Moderate risk because it changes provider update process spawning (Windows-specific command resolution) and alters the payload sent to Codex by dropping unsupported
serviceTiervalues, which could affect runtime behavior if assumptions were wrong.Overview
Fixes several Windows-specific runtime issues around provider maintenance and Codex requests.
Provider updates now resolve Windows command shims (e.g.
npm.cmd) viaresolveProviderMaintenanceSpawnCommandbefore spawning, and the serverbuildCLI stops forcingshellmode when runningnode --run build:bundle.Codex session startup/turn payloads now only forward supported
serviceTiervalues (fast/flex), omitting others to avoidthread/startandturn/startfailures; tests were added/updated accordingly. Desktop and script tests were hardened for Windows by normalizing path comparisons and skipping symlink-escape assertions when symlinks can’t be created on Windows, and oxlint rule tests now use a longer per-test timeout.Reviewed by Cursor Bugbot for commit dba5fae. Bugbot is set up for automated code reviews on this repo. Configure here.