Fix permission persistence failure due to Windows path case-sensitivity#2670
Fix permission persistence failure due to Windows path case-sensitivity#2670askadityapandey wants to merge 6 commits intoQwenLM:mainfrom
Conversation
Code Review for PR #2670The fix direction is correct — Windows file systems are case-insensitive but case-preserving, and Here are some suggestions: 1. (Medium) Inconsistent platform detectionThe PR uses two different styles for platform detection:
While functionally equivalent, it would be better to keep them consistent within the same PR. The existing codebase in 2. (Medium) Repeated pattern — consider extracting a shared utilityAll three files duplicate the same pattern: const isWindows = process.platform === 'win32';
const normalized = isWindows ? value.toLowerCase() : value;The project already has platform-aware path normalization in export function normalizePathForComparison(p: string): string {
return os.platform() === 'win32'
? path.normalize(p).toLowerCase()
: path.normalize(p);
}This would reduce duplication and make future maintenance easier. 3. (Medium) Missing unit testsThe PR does not include any tests. The existing test files for all three modules mock
4. (Low) Deleted useful comment in
|
| Severity | Count | Notes |
|---|---|---|
| Medium | 3 | Inconsistent platform detection, code duplication, missing tests |
| Low | 2 | Deleted comment, JSDoc not updated |
| Nit | 1 | Variable naming |
The core fix logic is sound. Main asks: add unit tests and consider a shared utility to reduce duplication. Thanks for the contribution!
|
pushed updates addressing all feedback:
let me know if anything else |
|
@askadityapandey It appears there are test errors. You can view the details of these errors by running |
done |
TL;DR
This PR fixes a bug on Windows 11 where permission options:
were not persisting between sessions.
Root cause:
Case-sensitive path comparisons on Windows (which is a case-insensitive file system).
process.cwd()andfs.realpathSync()can return paths with different casing (e.g.,C:\Users\Aditya\MyProjectvsC:\Users\Aditya\myproject), causing workspace settings to fail loading.Changes
packages/cli/src/config/settings.ts→ Case-insensitive comparison for workspace vs home directory check
packages/core/src/utils/fileUtils.ts→ Case-insensitive
isWithinRoot()function for Windowspackages/cli/src/config/trustedFolders.ts→ Case-insensitive
isPathTrusted()function for WindowsFixes
Closes #2669
Dive Deeper
Windows file systems are case-insensitive but case-preserving.
Node.js
fs.realpathSync()may return paths with different casing thanprocess.cwd(), even if they point to the same directory.The original code used strict equality (
===) for path comparison, which caused:Fix:
Normalize paths to lowercase on Windows before comparison to align with OS behavior.