Skip to content

fix(windows): refresh PATH ordering for newly installed tools - #13545

Open
OrcaWin wants to merge 3 commits into
stablyai:mainfrom
OrcaWin:OrcaWin/sta-3175-windows-repro
Open

fix(windows): refresh PATH ordering for newly installed tools#13545
OrcaWin wants to merge 3 commits into
stablyai:mainfrom
OrcaWin:OrcaWin/sta-3175-windows-repro

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #11992. Related to #12050; this branch reproduces the failure on current main and revalidates the fix on native Windows.

New Windows terminals inherited Orca's launch-time PATH and only appended missing persisted entries. Because executable lookup is first-match-wins, a stale WindowsApps alias stayed ahead of Python installed while Orca was running.

This change:

  • preserves launch-time and Orca-injected PATH entries that are absent from the registry;
  • adopts current machine/user registry ordering for persisted entries;
  • deduplicates case and trailing-separator variants without rewriting emitted paths;
  • leaves PATH untouched when persisted reads are empty or blocked;
  • invalidates the 30-second registry cache on WM_SETTINGCHANGE, with a generation guard so an in-flight read cannot restore stale data.

Screenshots

Final commit 8dc9c3f665 was launched on Windows Low with the inherited C:\Python314 entries deliberately removed while the live registry retained the post-install order. CDP identity matched this worktree, and the rendered terminal plus main-owned PTY buffer both reported Python at PATH index 15, WindowsApps at index 30, Get-Command python resolving to C:\Python314\python.exe, and Python 3.14.6.

Uncropped Orca window showing the fixed PATH order and executable resolution

Testing

  • pnpm run typecheck:node
  • pnpm test — the focused and affected suites below passed; the full matrix is running in CI.
  • pnpm build — local Electron dev build passed; release packaging is covered by CI.
  • Added regression coverage that fails against the previous implementation.

Results:

  • Regression-first result: 5/24 focused tests failed before the implementation, including Python index 3 after WindowsApps index 2.
  • windows-environment-path.test.ts + windows-path-registry-change.test.ts: 28/28 passed on the final rebased commit.
  • Six affected PATH/preflight/main-window suites: 158/158 passed.
  • Targeted oxlint --deny-warnings: passed.
  • pnpm run check:max-lines-ratchet: passed with no new bypasses.
  • Native Windows Electron scenario: passed visibly and in the main-owned PTY buffer.

AI Review Report

  • Cross-platform: both merge entry points and the native hook return immediately outside Windows; macOS and Linux behavior is unchanged.
  • Local, WSL, SSH, and folder workspaces: the change is confined to local Windows environment composition before PTY spawn and does not alter workspace discovery, provider selection, or remote execution.
  • Performance: the existing 30-second cache remains; there is no polling or extra per-terminal subprocess work. Invalidation is event-driven, and concurrent async refreshes remain deduplicated.
  • Failure handling: blocked/empty persisted reads preserve the working inherited PATH, and invalidation generations stop stale in-flight reads from repopulating the cache.
  • Path correctness: comparison is case-insensitive and trims trailing separators only for keys; emitted segments preserve their original spelling, and drive roots remain distinct from drive-relative paths.

Security Audit

  • No new command interpolation, network request, IPC field, dependency, or credential access is introduced.
  • Existing bounded reg.exe query calls and timeouts are unchanged.
  • Registry text is only reordered and deduplicated; it is never executed.
  • Making current persisted precedence authoritative narrows the window in which a stale alias can shadow a newly installed executable. Registry-unknown Orca/launch entries retain their existing priority.

Notes

  • The deliberate behavior change is that registry-known entries now follow current persisted precedence instead of stale inherited precedence; entries unknown to the registry retain inherited order ahead of them.
  • The real OS-wide WM_SETTINGCHANGE broadcast was not synthesized during validation to avoid changing registry or system-wide state. Listener installation, invalidation, and the in-flight race are covered deterministically in tests.
  • A pre-rebase low-spec renderer run needed one reload after ERR_INSUFFICIENT_RESOURCES while several unrelated dev servers were active. The final-commit run rendered without that error and passed end to end.

New terminals now preserve Orca-injected entries while adopting current machine and user PATH precedence, so newly installed tools are not shadowed by stale aliases. Invalidate the cached registry snapshot on Windows setting changes and guard in-flight refreshes from restoring stale cache data.
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@brennanb2025, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 47c1e901-eb73-459c-a45b-f4971696d1bb

📥 Commits

Reviewing files that changed from the base of the PR and between 30d8197 and 20edb4b.

📒 Files selected for processing (2)
  • src/main/pty/windows-environment-path.test.ts
  • src/main/pty/windows-environment-path.ts
📝 Walkthrough

Walkthrough

The change adds generation-based invalidation for the persisted Windows PATH cache and prevents stale asynchronous refreshes from repopulating it. PATH merging now normalizes, deduplicates, and orders injected and persisted entries. A Windows setting-change listener invalidates the cache on WM_SETTINGCHANGE. The listener is installed for the main window on supported Windows environments. Tests cover cache refreshes, PATH merging, registry messages, and unsupported platforms.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary Windows PATH ordering fix for newly installed tools.
Description check ✅ Passed The description includes all required sections and provides scope, testing results, review coverage, security notes, and platform-specific details.
Linked Issues check ✅ Passed The implementation meets issue #11992 by refreshing persisted PATH ordering, preserving injected entries, preventing stale aliases, and adding regression tests.
Out of Scope Changes check ✅ Passed The changes are limited to Windows PATH merging, cache invalidation, registry change handling, integration, and related tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

@OrcaWin

OrcaWin commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Native Windows low-spec evidence

Final commit 8dc9c3f665 was launched from this worktree with the inherited C:\Python314 entries deliberately removed while the live registry retained the post-install order. In a real new Orca pwsh tab, the rendered output and main-owned PTY buffer both reported Python at PATH index 15, WindowsApps at index 30, Get-Command python resolving to C:\Python314\python.exe, and Python 3.14.6.

Uncropped Orca window showing the fixed PATH order and executable resolution

The capture is uncropped and shows the branch/workspace context, real terminal surface, exact final commit, executable ordering, version output, and PASS verdict.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d6d1ce58-b617-4507-a5aa-aee2721f3753

📥 Commits

Reviewing files that changed from the base of the PR and between 8dc9c3f and 30d8197.

📒 Files selected for processing (2)
  • src/main/pty/windows-environment-path.test.ts
  • src/main/pty/windows-environment-path.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/pty/windows-environment-path.test.ts

Comment on lines +220 to +223
if (cacheGeneration !== persistedWindowsPathCacheGeneration) {
// Why: callers must not merge or inspect a snapshot invalidated while its queries ran.
return readPersistedWindowsPathSegmentsAsync({ forceRefresh: true })
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Prevent the retry from awaiting its own promise.

When the generation changes, Line 222 starts a forced refresh while pendingPersistedWindowsPathRefresh still references the current refresh promise. The pending-refresh check at Line 202 does not exclude forceRefresh, so the retry awaits its parent promise and never resolves.

Add !options.forceRefresh to the pending-refresh guard, or replace the pending promise before retrying. Add a regression test for invalidation during an active registry read.

Suggested fix
-  if (useProductionCache && pendingPersistedWindowsPathRefresh) {
+  if (!options.forceRefresh && useProductionCache && pendingPersistedWindowsPathRefresh) {
🧰 Tools
🪛 ast-grep (0.45.0)

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, execFileSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

@OrcaWin

OrcaWin commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Windows Electron QA — STA-3175 / PR #13545

Verdict: PASS on native Windows low-spec host
Tested head: 20edb4b41371cc79966ad7eacbc90e7bf0e3e16d (worktree HEAD matched PR headRefOid)
Method: Electron CDP via playwright-cli (no Computer Use / OS UI injection). Isolated dev profile; launch PATH deliberately stripped of C:\Python314* while machine/user registry retained post-install order (Python before WindowsApps).

Identity

  • CDP getIdentity: Orca: 20edb4b413, isDev: true, devLabel: sta-3175-windows-repro, devRepoRoot matched this worktree
  • Workspace sidebar showed Detached HEAD @ 20edb4b

User-visible PATH refresh / precedence (new Orca pwsh tab)

From rendered terminal + file write from the tab + main-owned PTY buffer (getMainBufferSnapshot, source: headless):

PATH[12]=C:\Python314\Scripts\
PATH[13]=C:\Python314\
PATH[28]=C:\Users\neil\AppData\Local\Microsoft\WindowsApps
Get-Command=C:\Python314\python.exe
python --version=Python 3.14.6
PythonIndex=13
WindowsAppsIndex=28
PASS=True
PATH_LEN=33

So persisted registry order is adopted for new terminals even when Orca's process PATH was stale at launch; first-match python resolves to the real install, not the Store alias.

Invalidation / generation-guard race

  • Focused unit tests: 28/28 pass
    windows-environment-path.test.ts + windows-path-registry-change.test.ts
    (includes cache invalidation on settings change and “current-generation refresh before merging an invalidated read”)
  • Gap: did not live-fire WM_SETTINGCHANGE by mutating the host registry during this session; race correction is covered by unit tests, not a live OS notification on this run.

Screenshots (uncropped real app window)

Full Orca window with PATH order and PASS=True

Same session visible PATH order + Get-Command + Python 3.14.6

Remaining risks

  1. Live WM_SETTINGCHANGE invalidation path not re-exercised end-to-end on this host.
  2. Validation used an isolated dev profile + folder-as-project open of the worktree, not a long-lived production profile.
  3. Agent-browser binary was not on PATH; automation used playwright-cli CDP attach as specified by the Electron skill.

Evidence branch (not on PR head): OrcaWin/orca@sta-3175-windows-qa-evidence / c954cbc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: New pwsh tabs keep stale Windows PATH ordering after installing Python

2 participants