fix(windows): adopt persisted PATH ordering in new terminals - #12050
fix(windows): adopt persisted PATH ordering in new terminals#12050ShreyanshVaibhaw wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughWindows PATH merging now normalizes entries for case-insensitive comparison and trailing-separator handling. It removes duplicates while preserving first occurrence order. The merge preserves inherited PATH entries when persisted data is empty, keeps injected entries before persisted entries, and rebuilds persisted ordering. Tests cover Python aliases, Orca-injected entries, registry read failures, ordering, and trailing separators. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6751804-07e2-413b-bc75-28bb6b192d13
📥 Commits
Reviewing files that changed from the base of the PR and between 4be4d10 and 84847590314af37b1274fa34599e07245730fd07.
📒 Files selected for processing (2)
src/main/pty/windows-environment-path.test.tssrc/main/pty/windows-environment-path.ts
| const trimmed = segment.replace(/[\\/]+$/, '') | ||
| return (trimmed.endsWith(':') ? segment : trimmed).toLowerCase() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize repeated separators on drive roots.
Line 248 preserves C:\\ as a different key from C:\. Windows treats both as the drive root. Deduplication can therefore retain duplicate root segments.
Proposed fix
- const trimmed = segment.replace(/[\\/]+$/, '')
- return (trimmed.endsWith(':') ? segment : trimmed).toLowerCase()
+ const trimmed = segment.replace(/[\\/]+$/, '')
+ const normalized = /^[a-z]:$/i.test(trimmed) && /[\\/]$/.test(segment)
+ ? `${trimmed}\\`
+ : trimmed
+ return normalized.toLowerCase()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const trimmed = segment.replace(/[\\/]+$/, '') | |
| return (trimmed.endsWith(':') ? segment : trimmed).toLowerCase() | |
| const trimmed = segment.replace(/[\\/]+$/, '') | |
| const normalized = /^[a-z]:$/i.test(trimmed) && /[\\/]$/.test(segment) | |
| ? `${trimmed}\\` | |
| : trimmed | |
| return normalized.toLowerCase() |
🧰 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)
Greptile SummaryThis PR fixes a Windows-specific PATH ordering bug where a new terminal opened inside a running Orca process could still resolve the old executable (e.g., the Windows Store Python alias) because the inherited PATH kept a stale entry ahead of a newly installed one. The fix rebuilds the merged PATH from persisted registry order rather than appending missing entries to the inherited order.
Confidence Score: 4/5Safe to merge; the change is Windows-only, well-guarded against registry failures, and covered by targeted regression tests that were verified to fail against the old implementation. The algorithm inversion is logically correct and the empty-persisted-list guard is an important safety net. The only gap found is that normalizeSegmentKey treats C:\ and C:/ as different drive-root keys so those two forms would not deduplicate against each other — a very unlikely real-world combination but worth fixing for completeness. Files Needing Attention: src/main/pty/windows-environment-path.ts — specifically the normalizeSegmentKey function; all other changed code is straightforward.
|
| Filename | Overview |
|---|---|
| src/main/pty/windows-environment-path.ts | Core implementation change: new normalizeSegmentKey/dedupeSegments helpers and a rewritten mergeWindowsPathSegments that rebuilds PATH from persisted registry order; logic is sound with good guards for edge cases |
| src/main/pty/windows-environment-path.test.ts | Test suite expanded from 12 to 16 cases; new tests cover the Python/WindowsApps regression, Orca-injected entry preservation, blocked registry, and trailing-separator dedup; existing concurrent-async test assertions remain valid under the new algorithm |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[mergePersistedWindowsPath called] --> B{platform === win32?}
B -- No --> Z[return early, PATH unchanged]
B -- Yes --> C[readPersistedWindowsPathSegments]
C --> D{Registry reads}
D -- All blocked/failed --> E[Return cached segments or empty list]
D -- Success --> F[Machine PATH segments + User PATH segments]
E --> G[mergeWindowsPathSegments]
F --> G
G --> H{persistedSegments.length === 0?}
H -- Yes --> Z
H -- No --> I[dedupeSegments on persisted list]
I --> J[Build persistedKeys set normalizeSegmentKey each]
J --> K[Filter currentSegments: keep only NOT in persistedKeys = injected entries]
K --> L[dedupeSegments on injected list]
L --> M[merged = injected + persisted]
M --> N{merged === currentPath?}
N -- Yes --> Z
N -- No --> O[env pathKey = merged: injected first then persisted order]
Reviews (1): Last reviewed commit: "fix(windows): adopt persisted PATH order..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const trimmed = segment.replace(/[\\/]+$/, '') | ||
| return (trimmed.endsWith(':') ? segment : trimmed).toLowerCase() |
There was a problem hiding this comment.
C:/) vs. backslash (C:\) produce different normalized keys here — 'c:/' and 'c:\\' — so they would not be deduplicated against each other. The PR comment correctly explains the C:\ vs C: distinction, but C:/ is a third form that the guard misses. In practice, C:/ as a bare PATH segment is rare, but if it did appear (e.g., from a POSIX-style tool on WSL) the injected-vs-persisted split would retain both entries.
| const trimmed = segment.replace(/[\\/]+$/, '') | |
| return (trimmed.endsWith(':') ? segment : trimmed).toLowerCase() | |
| const trimmed = segment.replace(/[\\/]+$/, '') | |
| // Normalise both `C:\` and `C:/` to the same key so they deduplicate correctly. | |
| return (trimmed.endsWith(':') ? trimmed + '\\' : trimmed).toLowerCase() |
Windows local
|
| Run | Tests | Test files |
|---|---|---|
Clean main at 4be4d10ae |
167 failed, 42573 passed, 516 skipped (43295) | 57 failed, 3988 passed, 28 skipped (4073) |
| This branch | 173 failed, 42571 passed, 516 skipped (43299) | 60 failed, 3985 passed, 28 skipped (4073) |
The branch total is 4 tests larger because this PR adds 4 tests.
The +6 / +3 delta is flake, not regression
Three test files fail on the branch run that did not fail on the main run. No file fails on main that passes on the branch. I investigated each of the three individually rather than assuming:
| File | Finding |
|---|---|
src/main/native-chat/transcript-watch-liveness.test.ts |
Flaky on unmodified main. Run three times in isolation on a clean main checkout: passed, failed, passed. |
src/renderer/.../project-view-wrapper-source-context-boundary.test.ts |
Fails on main too. Reproduces in isolation on clean main with a 30s timeout (30015ms). |
src/main/codex-usage/scanner-paths.test.ts |
Passes in isolation on this branch. Only fails under full-suite parallel load. |
None of the three touch PATH construction, PTY spawn, or the registry. The change is confined to mergeWindowsPathSegments, guarded by platform !== 'win32' early returns in both public entry points, so macOS and Linux never reach the modified code.
Why the pre-existing failures matter for review
The failures on main cluster in exactly the area this PR touches:
src/main/ipc/pty.test.tssrc/main/daemon/pty-subprocess.test.tssrc/relay/pty-handler.test.ts
All three fail on unmodified main. The remainder are broadly environmental for a local Windows box: src/main/ssh/*, src/main/startup/*, src/main/ipc/ephemeral-vm.test.ts, src/relay/*, plus tests that assume a POSIX shell. Two representative errors:
Error: spawn /bin/sh ENOENT
spawnargs: [ '-c', '"C:\Program Files\nodejs\node.exe" ... ' ]
Error: Timed out waiting for PTY process exit: pty-1
at src/relay/pty-handler.ts:1747
src/main/pty/windows-environment-path.test.ts does not appear in either failure list.
Targeted results for the code this PR changes
src/main/pty/windows-environment-path.test.ts: 16 passed (up from 12)src/main/pty/plussrc/main/ipc/preflight: 141 passed, 17 skipped, 0 failedoxlinton both changed files: cleanpnpm typecheck: passes- max-lines ratchet: OK, no new bypasses
The new tests were confirmed to fail against the previous implementation, so they catch the regression rather than merely passing alongside the fix. Also verified end to end on Windows 11 against the live registry and real process.env, using a scratch test that was not committed.
One pre-existing lint failure
pnpm lint fails at verify:skill-bundle-manifest with stale resources/skills/*.json. This also reproduces on a clean checkout of main, so it is not introduced here. I left those generated artifacts alone to keep this diff focused. Happy to open a separate issue if useful.
I am not presenting these as a passing run. They are here so the unticked checkbox is not mistaken for an untested change, and so the Windows-local baseline is on the record. CI remains the authority.
New Windows terminals inherited Orca's launch-time PATH and only had missing persisted segments appended to the tail. Because Windows executable resolution is first-match-wins, a stale `WindowsApps` alias kept shadowing a directory the registry now orders ahead of it, so a Python installed while Orca was running resolved to the Store stub that reports Python as missing. Rebuild the merged PATH from the persisted machine/user values instead of appending to them: - Persisted segments keep their registry order. - Segments the registry does not know about (Orca-injected, or added by the launching shell) are preserved in inherited order ahead of the persisted list, so Orca's own tools still win. - An empty, blocked, or timed-out registry read leaves PATH untouched. - Segments differing only by a trailing separator no longer duplicate. The previous append-only behavior was deliberate and documented as avoiding reordering, so this reverses that decision and rewrites the test that encoded it. The issue's acceptance criteria ask for persisted ordering to win. Verified on Windows 11 against the live registry, and confirmed the new tests fail against the previous implementation. Fixes stablyai#11992
8484759 to
c0d23dc
Compare
|
Thank you for the careful Windows PATH work and the detailed baseline investigation. We ran a fresh native Windows comparison of this PR against #13545 on current main. Both changes fix ordinary PATH ordering, but this implementation can still merge a registry snapshot after that snapshot has been invalidated: the controlled race returned the stale |
4 participants
Summary
Fixes #11992.
On Windows, a terminal opened after installing a tool could still resolve the old executable. Installing Python while Orca was running left new pwsh tabs resolving
pythonto theWindowsAppsStore alias, which reports that Python is not installed, while a pwsh launched outside Orca resolved the real interpreter.mergeWindowsPathSegmentsread the persisted machine/user PATH from the registry but only appended the segments that were missing from Orca's inherited PATH. Windows executable resolution is first-match-wins, so an inheritedWindowsAppsentry stayed ahead of aPython314\directory that the persisted PATH now orders before it.This rebuilds the merged PATH from the persisted values instead of appending to them:
reg.exequery can never rewrite a working PATH.Deliberate behavior change, please review
The previous append-only behavior was intentional. The comment on
mergePersistedWindowsPathdocumented it as appending "without unexpectedly reordering existing PATH", andmergePersistedWindowsPathhad a test named "appends missing persisted segments without reordering the inherited PATH" asserting exactly that.This PR reverses that decision and rewrites that test, because the two goals are mutually exclusive: you cannot both preserve the inherited order and stop a stale entry from shadowing a newer one. The issue's acceptance criteria ask for persisted ordering to win. Flagging it explicitly so the tradeoff is a decision rather than an accident.
Screenshots
No visual change.
Testing
pnpm lint(one pre-existing unrelated failure, see Notes)pnpm typecheckpnpm test(full suite did not complete locally, see Notes)pnpm build(not run locally)src/main/pty/windows-environment-path.test.tsgoes from 12 to 16 tests:stops an inherited WindowsApps alias from shadowing a newly installed Pythonis the regression test the issue asks for.keeps Orca-injected entries the registry does not know about ahead of persisted onespins the constraint that Orca's own PATH entries survive.leaves the inherited PATH untouched when both registry reads are blockedcovers the denied-registry path.does not duplicate a segment that differs only by a trailing separatorcovers the dedup fix.adopts persisted machine and user PATH orderingreplaces the test that asserted the old append-only contract.These tests were verified to fail against the previous implementation. Reverting the source and re-running produces 4 failures, including
expected 3 to be less than 2for the Python/WindowsApps ordering, so they genuinely catch the regression rather than merely passing alongside the fix.Also verified end to end on Windows 11 against the live registry, using a temporary test that ran
mergePersistedWindowsPathover the realprocess.envand realreg.exeoutput. It confirmed the resulting PATH preserves the deduplicated persisted ordering on real data. That scratch file was not committed.Suite results with the change applied:
src/main/pty/windows-environment-path.test.ts: 16 passedsrc/main/pty/plussrc/main/ipc/preflight: 141 passed, 17 skipped, 0 failedoxlinton both changed files: cleanAI Review Report
Reviewed with Claude. Risks checked:
platform !== 'win32', so behavior on macOS and Linux is unchanged. No shortcuts, labels, or UI strings are touched. Path handling stays string-level on PATH segments and does not introduce separator assumptions beyond the existinggetPathDelimiterhelper. No Electron-specific APIs are touched.preflight-local-env.ts,preflight.test.ts,preflight-agent-detection-no-subprocess.test.ts) either mock the function or do not assert ordering.preflight-windows-path-refresh.repro.test.tsexercises the real implementation and still passes, since it only asserts that a newly persisted directory becomes resolvable.C:\andC:differ on Windows;normalizeSegmentKeyguards that case.Security Audit
reg.exe querycalls are unchanged, including their 5s timeout andwindowsHide.normalizeSegmentKeyis used strictly as a comparison key. Emitted segments keep their original spelling from the registry or the inherited PATH, so no path is rewritten or silently canonicalized.Notes
pnpm lintfails atverify:skill-bundle-manifestwith staleresources/skills/*.jsonartifacts. This reproduces on a clean checkout ofmainwith no changes applied, so it is not caused by this PR. I deliberately did not regenerate those artifacts here, since they are generated files and would add unrelated churn to a focused fix. Happy to open a separate issue if useful. Every other lint stage passes, including the type-aware pass, reliability gates, and the max-lines ratchet.pnpm testandpnpm buildwere not completed locally. The full suite ran for over 35 minutes without finishing on this machine and was not a clean measurement, so I am not claiming it passed. The directly affected suites pass, as listed above. CI coverage is welcome here.keepLastGoodSegmentslogic already distinguishes from a blocked read.