Skip to content

Fix permission persistence failure due to Windows path case-sensitivity#2670

Open
askadityapandey wants to merge 6 commits intoQwenLM:mainfrom
askadityapandey:main
Open

Fix permission persistence failure due to Windows path case-sensitivity#2670
askadityapandey wants to merge 6 commits intoQwenLM:mainfrom
askadityapandey:main

Conversation

@askadityapandey
Copy link
Copy Markdown

TL;DR

This PR fixes a bug on Windows 11 where permission options:

  • "Always allow in this project"
  • "Always allow for this user"

were not persisting between sessions.

Root cause:
Case-sensitive path comparisons on Windows (which is a case-insensitive file system).
process.cwd() and fs.realpathSync() can return paths with different casing (e.g., C:\Users\Aditya\MyProject vs C:\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 Windows

  • packages/cli/src/config/trustedFolders.ts
    → Case-insensitive isPathTrusted() function for Windows


Fixes

Closes #2669


Dive Deeper

Windows file systems are case-insensitive but case-preserving.

Node.js fs.realpathSync() may return paths with different casing than process.cwd(), even if they point to the same directory.

The original code used strict equality (===) for path comparison, which caused:

  1. Workspace settings not loading (incorrectly treated as home directory)
  2. Path trust checks failing
  3. Permissions appearing lost between sessions

Fix:
Normalize paths to lowercase on Windows before comparison to align with OS behavior.


@LaZzyMan
Copy link
Copy Markdown
Collaborator

Code Review for PR #2670

The fix direction is correct — Windows file systems are case-insensitive but case-preserving, and process.cwd() / fs.realpathSync() can indeed return paths with different casing. Using toLowerCase() on Windows before comparison is the right approach.

Here are some suggestions:


1. (Medium) Inconsistent platform detection

The PR uses two different styles for platform detection:

  • settings.ts: platform() (imported from node:os)
  • trustedFolders.ts & fileUtils.ts: process.platform

While functionally equivalent, it would be better to keep them consistent within the same PR. The existing codebase in paths.ts uses os.platform(), so aligning to that style is preferable.

2. (Medium) Repeated pattern — consider extracting a shared utility

All 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 packages/core/src/utils/paths.ts (getProjectHash, sanitizeCwd, isSubpath). Consider extracting a shared utility function like:

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 tests

The PR does not include any tests. The existing test files for all three modules mock platform as 'linux' and have no Windows-specific test cases. At minimum, please add tests for:

  • isWithinRoot: Verify case-insensitive matching when process.platform is mocked as 'win32'
  • isPathTrusted: Verify untrusted path matching is case-insensitive on Windows
  • loadSettings: Verify isWorkspaceSameAsHome correctly identifies mixed-case home/workspace dirs on Windows

4. (Low) Deleted useful comment in fileUtils.ts

The original comment explaining rootWithSeparator was removed:

// Ensure the rootDirectory path ends with a separator for correct startsWith comparison,
// unless it is the root path itself (e.g., '/' or 'C:\\').

Please keep this comment and add a new one explaining the case-normalization logic for Windows.

5. (Low) JSDoc not updated for isWithinRoot

The function's JSDoc does not mention the new case-insensitive behavior on Windows. A brief note like "On Windows, comparison is case-insensitive to match OS behavior." would help future readers.

6. (Nit) Variable naming in isWithinRoot

rootToCompare vs normalizedRootToCompare can be confusing — one includes the trailing separator and the other does not. Consider renaming to rootWithSepToCompare and rootDirToCompare for clarity.


Summary

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!

@askadityapandey
Copy link
Copy Markdown
Author

pushed updates addressing all feedback:

  • unified platform detection
  • extracted normalizePathForComparison into shared util
  • added windows-specific tests across settings, trusted folders, and file utils
  • restored comments and improved naming

let me know if anything else

@LaZzyMan
Copy link
Copy Markdown
Collaborator

@askadityapandey It appears there are test errors. You can view the details of these errors by running npm test:ci on the test page or locally.

@askadityapandey
Copy link
Copy Markdown
Author

@askadityapandey It appears there are test errors. You can view the details of these errors by running npm test:ci on the test page or locally.

done

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.

Permission options 2 and 3 not persisting in Windows 11 CMD (re-prompts every time)

2 participants