fix(windows): cache rooted path probes and UNC globs#582
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors Windows path handling to use fs.realpathSync.native, constructs Windows-style wildcard patterns with win32.join, introduces an optional cache plus internal cache-invalidating logic for rooted-but-driveless path resolution, adjusts external-directory dirname selection on Windows, and adds Windows-only tests for UNC/extended UNC and caching behavior. ChangesWindows Path Normalization and Caching
Possibly related issues
Possibly related PRs
Suggested labels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Astro-Han
left a comment
There was a problem hiding this comment.
GLM First-pass Review
P0-P3 Severity Analysis: No P0/P1/P2/P3 issues found.
Spec Verification
| Spec Item | Implementation | Status |
|---|---|---|
| Rooted-driveless cache hit logic | cache.get + exists check | Covered |
| Cache invalidation on target disappearance | cache.delete when !exists | Covered |
| Cache invalidation on cwd root change | refreshRootedWindowsVariantCache compares cwdRoot | Covered |
| Cache invalidation on SystemDrive change | envKey includes systemDrive | Covered |
| UNC share root preserved in glob | path.win32.dirname/join for UNC paths | Covered |
| Extended UNC glob normalization | Test verifies UNC path handling | Covered |
| Windows-only (no macOS/Linux impact) | process.platform !== win32 early return | Covered |
Test Coverage
- Cache reuse: normalize-path.test.ts verifies probe count reduction
- Cache invalidation: Test verifies refreshed result after live.set
- Extended UNC: normalizePathPattern test
- UNC glob: external-directory.test.ts verifies patterns/always preserve share root
Summary
- 4 files +141/-8, minimal scope
- Cache hit/invalidation paths complete
- UNC glob correctly preserves share root
- Tests cover all 4 boundary cases
- No macOS/Linux path behavior changes
No blocking issues. Ready for second-pass review.
There was a problem hiding this comment.
Code Review
This pull request introduces a caching mechanism for resolving rooted-but-driveless Windows paths to improve performance and ensures consistent use of win32 path methods and fs.realpathSync.native for better Windows compatibility, especially regarding UNC paths. Corresponding tests were added to verify these changes. Feedback was provided to simplify the cache key generation by removing a redundant string replacement that is already handled by the path normalization function.
Astro-Han
left a comment
There was a problem hiding this comment.
Second-pass (product / scope) review.
Three-question check passes: change stays narrow (4 files +141/-8), cache invalidation covers cached-target removal / cwd-root change / SystemDrive change, UNC glob fix routes consistently through path.win32, and resetWindowsRootedVariantCacheForTest is test-only.
One P3 clarification, non-blocking:
packages/core/src/filesystem.ts resolve() switched its realpath call from realpathSync(resolved) to fs.realpathSync.native(resolved). This isn't called out in the PR Review Focus or in issue #497's spec. It looks like an intentional consistency tweak (so resolve() matches canonicalize() which already used .native), but please confirm:
- Is this intentional, not an unintended side effect of the
import * as fs from "fs"refactor? - Are the existing tests covering
.nativebehavior on Windows for symlink / junction / ENOENT / non-existent paths, given.nativeand the JS-shim variant can diverge on edge cases?
If intentional and behaviorally equivalent for the cases we care about, this is fine to keep as a small scope-aligned perf nudge. If accidental, please revert that single line to keep PR scope tight.
No P0 / P1 / P2 findings. Awaiting GPT-X engineering final review.
554e12b to
725854b
Compare
725854b to
fb121ed
Compare
Summary
Cache repeated Windows rooted-but-driveless canonicalization probes and close the remaining UNC validation gap in the external-directory permission path.
Changes included:
path.win32so\\server\share\...permission scopes do not collapse to*Why
Issue #497 left two bounded Windows follow-ups after #493:
This PR keeps the fix inside that P3 follow-up surface. It does not reopen #427 correctness work, wildcard semantics, or broader path architecture.
Related Issue
Closes #497.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Risk Notes
How To Verify
Screenshots or Recordings
None. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Bug Fixes
Performance
Tests