fix(desktop): skip slow powershell profile load if node is available

[mohamedmastouri-hue]

Fixes #4403 by skipping the slow PowerShell profile load if node.exe is already found in the no-profile path.

---

Note

Medium Risk
Changes Windows startup PATH/FNM resolution; conservative fallbacks re-run PowerShell on profile read errors or missing USERPROFILE, but a false negative on fnm detection could omit FNM env vars until probes run.

Overview
Windows desktop startup can skip both PowerShell environment probes when node.exe is already discoverable from the current PATH plus knownWindowsCliDirs, and no PowerShell profile text mentions fnm.

Before spawning PowerShell, installWindowsEnvironment runs parallel filesystem checks (250ms caps): scan merged static PATH entries for node.exe (including quoted segments) and read standard/OneDrive profile paths for an fnm reference (UTF-8/UTF-16). On the fast path it sets PATH from that static merge and returns; otherwise it keeps the existing concurrent no-profile + profile probes and still merges FNM_* from the profile run.

Also adds ~\.cargo\bin to known Windows CLI dirs. Tests gain injectable FileSystem and cases covering skip vs probe when fnm appears in normal or OneDrive profiles.

^{Reviewed by Cursor Bugbot for commit 23081ba. Bugbot is set up for automated code reviews on this repo. Configure here.}

Note

Skip PowerShell profile load on Windows startup when node.exe is found on static paths

• On Windows, installWindowsEnvironment now checks for node.exe in known static CLI dirs (PATH + knownWindowsCliDirs) and scans PowerShell profiles for fnm references before spawning any PowerShell processes.
• If node is found and no profile references fnm, both PowerShell probes are skipped entirely and PATH is set from static sources, avoiding slow profile load on startup.
• If node is found but a profile references fnm (including OneDrive-redirected profiles), the existing PowerShell probes still run to pick up FNM_DIR.
• Adds ~\.cargo\bin to knownWindowsCliDirs so Rust toolchain binaries are included in the merged PATH.
• installWindowsEnvironment now requires a FileSystem.FileSystem service in its effect environment for existence checks and profile reads (with 250ms timeouts).

^{Macroscope summarized 23081ba.}

[coderabbitai]

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 Plus

Run ID: 6396040f-e0a5-450a-89b1-232e3bce700b

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

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[macroscopeapp]

One finding on the Windows shell-environment fast path: the {} placeholder for a skipped profile probe drops the EnvironmentPatch type and silently disables FNM hydration, and this backend behavior change has no focused test.

Convention referenced: If backend behavior changes, require focused tests (change discipline). The existing Windows test still exercises only the profile-loading path, because it runs against the real NodeServices filesystem where ...\node.exe never exists; the new skip branch is untested.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This PR introduces conditional logic that skips PowerShell profile loading when node.exe is found statically, changing what environment variables get loaded at startup. The behavior change is substantive and there's an open Medium-severity finding about unhandled edge cases for profile locations.

^{You can customize Macroscope's approvability policy. Learn more.}

[macroscopeapp]

Two issues in the new Windows startup tests: the added cases are currently unreachable (never registered with the test runner), and once registered they depend on real filesystem lookups with Windows path separators, so they cannot pass on non-Windows CI. Since this PR changes Windows startup behavior, the focused tests need to actually run and be platform-independent.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One retained finding after the restructure into Effect.all: the skipped-profile branch still yields an untyped {}, which widens the tuple element and breaks the later profile.PATH / profile.FNM_DIR reads. Details inline.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

Effect service conventions review: one change-discipline finding on the removed concurrency rationale. The service definition, layer, dependency acquisition (yield* FileSystem.FileSystem), and error modeling all look consistent with the conventions, and the new tests are now registered at describe level and platform-independent.

^{Posted via Macroscope — Effect Service Conventions}

Dismissing prior approval to re-evaluate 989bfa7

[macroscopeapp]

One finding on the new Windows fast-path tests: the knownWindowsCliDirs case does not exercise the path it claims to. Production code changes look consistent with the service conventions (dependency acquired via yield* FileSystem.FileSystem, namespace imports, preserved concurrency rationale).

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One finding: the fast-path pre-check is fine as an Effect service change (dependency acquired via yield* FileSystem.FileSystem, requirement surfaced in the Effect.fn.Return type, make/layer unchanged), but the Windows path-separator rewrite in knownWindowsCliDirs changes production behavior beyond the perf fix.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One convention issue: the new FileSystem usages in the test file have no corresponding namespace import.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One import-convention violation found in the changed test file. See the inline comment.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One convention issue found: the test provides a FileSystem tag from a different package than the service under test requires.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One finding in the test harness: the conditional Effect.provideService leaves FileSystem.FileSystem in the helper's declared requirements, so every it.effect call site in this file no longer typechecks.

^{Posted via Macroscope — Effect Service Conventions}

[CDVolvik]

The early return skips the profile probe, and that probe is also the only source of FNM_DIR and FNM_MULTISHELL_PATH:

readWindowsEnvironment(WINDOWS_PROFILE_ENV_NAMES, { loadProfile: true })
// WINDOWS_PROFILE_ENV_NAMES = ["PATH", "FNM_DIR", "FNM_MULTISHELL_PATH"]

Both feed the tail of installWindowsEnvironment, which the fast path never reaches. The !fnmFound guard is meant to cover this, but it only scans knownWindowsCliDirs plus the static PATH. If fnm is installed outside those (cargo install fnm lands in %USERPROFILE%\.cargo\bin, which isn't in knownWindowsCliDirs), fnmFound stays false, and any statically discoverable node.exe then triggers the skip.

Repro as a standalone test file, run against both revisions:

// node.exe is statically discoverable on PATH.
// fnm is installed somewhere the scan never looks.
const nodeDir = "C:\\Program Files\\nodejs";
const env: NodeJS.ProcessEnv = { PATH: nodeDir };

const mockFs = {
  exists: (path: string) => Effect.succeed(path.includes("node.exe")),
} as FileSystem.FileSystem;

yield* runShellEnvironment({
  env,
  platform: "win32",
  fs: mockFs,
  handler: (command) => {
    const noProfile =
      command._tag === "StandardCommand" && command.args.includes("-NoProfile");
    return noProfile
      ? envOutput({ PATH: nodeDir })
      : envOutput({
          PATH: `${nodeDir};C:\\Users\\dev\\.fnm\\aliases\\default`,
          FNM_DIR: "C:\\Users\\dev\\.fnm",
          FNM_MULTISHELL_PATH: "C:\\Users\\dev\\AppData\\Local\\fnm_multishells\\1234",
        });
  },
});

assert.equal(env.FNM_DIR, "C:\\Users\\dev\\.fnm");

On main it passes. On this branch FNM_DIR is undefined. Your own three new tests are 12/12 green, so this is uncovered rather than broken by them.

FNM_MULTISHELL_PATH is the harder half: fnm sets it in the shell profile, so no filesystem scan can recover it.

Smaller things:

• The two probes this replaces are each bounded by LOGIN_SHELL_TIMEOUT (5s). The scan is bounded by nothing. It is serial (yield* inside the loop), up to four exists calls per PATH entry, and in the common no-fnm case if (nodeFound && fnmFound) break never fires, so it walks every entry. A stale mapped network drive on PATH can stall startup longer than the two PowerShell spawns did.
• Effect.orElseSucceed(fileSystem.exists(...), () => false) makes a permissions error read as "not present", which flips skipProfile the unsafe way.
• ${cleanDir}/node.exe mixes separators against paths built with \ elsewhere in the file.
• None of the three new tests covers fnm present but outside the scanned dirs, which is exactly where the guard gives way.

Direction looks right to me. Skipping both probes when nothing needs them is a bigger win than reordering the merge, and the concurrency change landed in #5878 already took most of the value out of skipping just one. The piece that needs rework is deciding the fnm case from something other than a filesystem scan of directories fnm may not live in.

[macroscopeapp]

🟡 Medium shell/DesktopShellEnvironment.ts:426

When the actual PowerShell profile is outside the four hard-coded %USERPROFILE%\Documents paths, this check returns false; if a static node.exe is found, skipProfile then skips both PowerShell probes and leaves fnm's selected Node and environment out of PATH. Treat an unresolved profile location conservatively as unsafe to skip (or resolve the Windows Documents known folder).

Suggested change

	return false;
	return true;

🤖 Copy this AI Prompt to have your agent fix this:

In file @apps/desktop/src/shell/DesktopShellEnvironment.ts around line 426:

When the actual PowerShell profile is outside the four hard-coded `%USERPROFILE%\Documents` paths, this check returns `false`; if a static `node.exe` is found, `skipProfile` then skips both PowerShell probes and leaves fnm's selected Node and environment out of `PATH`. Treat an unresolved profile location conservatively as unsafe to skip (or resolve the Windows Documents known folder).

[mohamedmastouri-hue]

Addressed in 23081ba: checkProfileMentionsFnm now builds the probe list from %USERPROFILE%\Documents plus %OneDrive%\Documents (expanded from the OneDrive env var) when set, so Known Folder Move / OneDrive-redirected $PROFILE locations are inspected before deciding it is safe to skip the PowerShell probes. No processes are spawned; the whole check stays bounded by the existing 250ms timeout. A regression test covers fnm referenced only under the OneDrive-redirected path. Remaining gap: redirects to arbitrary non-OneDrive locations would still need a known-folder resolution, which is out of scope here.

[macroscopeapp]

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

[macroscopeapp]

Reviewed the new Windows fast-path against the Effect service conventions. The service definition/layer shape is fine (FileSystem is acquired from the environment and surfaced in Effect.fn.Return), but the new error-handling code uses APIs/predicates that do not exist in this repo's Effect v4 line, and two of the new tests cannot pass as written.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

The predicate updated in the latest commit still does not identify a missing PowerShell profile the way the repo's other PlatformError handlers do, and the two dead as any clauses hide that. Previously flagged items on this change (Effect.catchAll at lines 396 and 428, and the PlatformError.systemError arguments in the new tests) are unchanged and still apply.

^{Posted via Macroscope — Effect Service Conventions}

[cursor]

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

There are 3 total unresolved issues (including 2 from previous reviews).

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

^{Reviewed by Cursor Bugbot for commit b3b27d6. Configure here.}

[macroscopeapp]

Two items in the Windows fast-path change:

1. apps/desktop/src/shell/DesktopShellEnvironment.ts:396 and :428 still use Effect.catchAll, which does not exist in Effect v4 (effect@4.0.0-beta.103). Every other call site in this repo uses Effect.catch, and for probes collapsed to a boolean the file's own idiom is Effect.orElseSucceed (see line 555). Flagged in an earlier run and unchanged.
2. The FileSystem stubs fabricate a PlatformError with as any instead of the real constructor — inline comment below.

Nothing else in the diff conflicts with the service conventions: installWindowsEnvironment acquires FileSystem.FileSystem from the environment and surfaces it in its Effect.fn.Return requirements, the catchIf on reason._tag === "NotFound" is a legitimate structural platform-code check, and the concurrency rationale comment was restored.

^{Posted via Macroscope — Effect Service Conventions}

[macroscopeapp]

One finding: the new Windows fast-path test asserts PATH is left untouched, but the fast path rewrites it with the merged static paths. Everything else (namespace subpath imports, FileSystem.FileSystem acquired from the environment in make, the structural reason._tag catchIf, and the typed PlatformError.systemError stubs) now matches the repository's Effect service conventions.

^{Posted via Macroscope — Effect Service Conventions}

[CDVolvik]

The new revision fixes both of my 8/14 findings — .cargo\bin is now in knownWindowsCliDirs, and the skip is gated on the profile not mentioning fnm, with fail-safe defaults on read errors and timeouts. Nice response.

One regression class survives, though, and it's the same shape: the skip path drops non-fnm profile PATH prepends that main preserves.

Scenario: a user's Microsoft.PowerShell_profile.ps1 contains $env:Path = "C:\mytools;$env:Path" (no fnm anywhere), and node.exe is in a standard location. Then nodeFound && !profileMentionsFnm ⇒ skip ⇒ config.env.PATH = staticPaths — and C:\mytools is gone. On main, the loadProfile: true probe captures it (profile.PATH is merged first). Any provider CLI that lives only in a profile-prepended dir stops resolving after this PR.

Actionable version: the cheap gate that doesn't need to execute the profile is "every candidate profile file is absent or empty" — you already read the bytes for the fnm check, so bytes.length > 0 for any profile ⇒ it may mutate PATH ⇒ don't skip. That keeps the 0ms win for the (very common) no-profile case, which is the case the linked issue is actually about, without changing behavior for anyone whose profile does real work.