fix: prefer native realpath for canonical paths#972
Conversation
📝 WalkthroughWalkthroughThe PR refactors filesystem API imports to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Looks good. Native realpath is the right fix here, and the regression test covers the intended behavior.
![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
🧹 Nitpick comments (1)
test/utils/file-system.test.ts (1)
96-110: Add fallback-branch regression tests for full canonicalization contract.This covers the native path, but the updated logic also depends on two fallbacks. Please add tests for:
nativethrows →realpathSyncis used,- both throw →
path.resolveis returned.
That will lock in the behavior introduced insrc/utils/file-system.tsLine 65-Line 69.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/file-system.test.ts` around lines 96 - 110, Add two regression tests for FileSystemUtils.canonicalizeExistingPath: one where nodeFs.realpathSync.native is made to throw (spy to throw), assert that nodeFs.realpathSync was called and its result returned; and one where both nodeFs.realpathSync.native and nodeFs.realpathSync are made to throw, assert that path.resolve was used and its value returned. In each test, create a temporary file like the existing test, use vi.spyOn on nodeFs.realpathSync.native and nodeFs.realpathSync to simulate throws or returns, call FileSystemUtils.canonicalizeExistingPath(filePath), assert the correct fallback behavior (calls and return values), and restore mocks after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/file-system.test.ts`:
- Around line 101-108: The test creates a spy on nodeFs.realpathSync.native and
manually restores it with nativeSpy.mockRestore, but restoration can be skipped
if assertions throw; wrap the spy usage around the call to
FileSystemUtils.canonicalizeExistingPath and the expect assertions in a
try/finally block and call nativeSpy.mockRestore() in the finally to guarantee
cleanup even on failure.
---
Nitpick comments:
In `@test/utils/file-system.test.ts`:
- Around line 96-110: Add two regression tests for
FileSystemUtils.canonicalizeExistingPath: one where nodeFs.realpathSync.native
is made to throw (spy to throw), assert that nodeFs.realpathSync was called and
its result returned; and one where both nodeFs.realpathSync.native and
nodeFs.realpathSync are made to throw, assert that path.resolve was used and its
value returned. In each test, create a temporary file like the existing test,
use vi.spyOn on nodeFs.realpathSync.native and nodeFs.realpathSync to simulate
throws or returns, call FileSystemUtils.canonicalizeExistingPath(filePath),
assert the correct fallback behavior (calls and return values), and restore
mocks after each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61ab8a0d-371a-4b68-9447-d20ac3ea6f48
📒 Files selected for processing (2)
src/utils/file-system.tstest/utils/file-system.test.ts
| const nativeSpy = vi.spyOn(nodeFs.realpathSync, 'native'); | ||
|
|
||
| const resolved = FileSystemUtils.canonicalizeExistingPath(filePath); | ||
|
|
||
| expect(nativeSpy).toHaveBeenCalledWith(filePath); | ||
| expect(resolved).toBe(nodeFs.realpathSync.native(filePath)); | ||
|
|
||
| nativeSpy.mockRestore(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking Vitest config for automatic mock restoration..."
fd -i 'vitest.config.*' | while read -r file; do
echo "---- ${file} ----"
rg -n -C2 'restoreMocks|clearMocks|mockReset' "$file" || true
done
echo
echo "Checking test setup hooks for global mock cleanup..."
rg -n -C2 "afterEach\\(.*vi\\.(restoreAllMocks|resetAllMocks|clearAllMocks)" --type ts --type js || trueRepository: Fission-AI/OpenSpec
Length of output: 200
🏁 Script executed:
# Check the full vitest.config.ts file
cat vitest.config.tsRepository: Fission-AI/OpenSpec
Length of output: 1419
🏁 Script executed:
# Search for any setup files referenced in config or at standard locations
fd -i 'setup.*\.(ts|js)$' | head -20Repository: Fission-AI/OpenSpec
Length of output: 78
🏁 Script executed:
# Search for afterEach hooks in all test files to see if there's a pattern of manual cleanup
rg -n "afterEach\(" test/ --type ts | head -10Repository: Fission-AI/OpenSpec
Length of output: 664
🏁 Script executed:
# Check the actual test file context around the code snippet
cat test/utils/file-system.test.ts | head -120 | tail -40Repository: Fission-AI/OpenSpec
Length of output: 1436
🏁 Script executed:
# Check vitest.setup.ts content
cat vitest.setup.tsRepository: Fission-AI/OpenSpec
Length of output: 542
🏁 Script executed:
# Get the afterEach hook at line 17 in the test file with context
sed -n '15,30p' test/utils/file-system.test.tsRepository: Fission-AI/OpenSpec
Length of output: 520
🏁 Script executed:
# Get the full import section and context around the spy test
sed -n '1,25p' test/utils/file-system.test.tsRepository: Fission-AI/OpenSpec
Length of output: 863
🏁 Script executed:
# Get the full test block around the spy test to see if there are multiple tests that could be affected
sed -n '96,125p' test/utils/file-system.test.tsRepository: Fission-AI/OpenSpec
Length of output: 1161
Wrap spy cleanup in try/finally for guaranteed mock restoration.
Vitest auto-restore is not enabled in this project's configuration. The spy created on line 101 is manually restored on line 108, but if any assertion fails before then, the cleanup will not run, leaving the mock in a dirty state. Use try/finally to guarantee restoration:
Suggested patch
- const nativeSpy = vi.spyOn(nodeFs.realpathSync, 'native');
-
- const resolved = FileSystemUtils.canonicalizeExistingPath(filePath);
-
- expect(nativeSpy).toHaveBeenCalledWith(filePath);
- expect(resolved).toBe(nodeFs.realpathSync.native(filePath));
-
- nativeSpy.mockRestore();
+ const nativeSpy = vi.spyOn(nodeFs.realpathSync, 'native');
+ try {
+ const resolved = FileSystemUtils.canonicalizeExistingPath(filePath);
+ expect(nativeSpy).toHaveBeenCalledWith(filePath);
+ expect(resolved).toBe(nodeFs.realpathSync.native(filePath));
+ } finally {
+ nativeSpy.mockRestore();
+ }📝 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 nativeSpy = vi.spyOn(nodeFs.realpathSync, 'native'); | |
| const resolved = FileSystemUtils.canonicalizeExistingPath(filePath); | |
| expect(nativeSpy).toHaveBeenCalledWith(filePath); | |
| expect(resolved).toBe(nodeFs.realpathSync.native(filePath)); | |
| nativeSpy.mockRestore(); | |
| const nativeSpy = vi.spyOn(nodeFs.realpathSync, 'native'); | |
| try { | |
| const resolved = FileSystemUtils.canonicalizeExistingPath(filePath); | |
| expect(nativeSpy).toHaveBeenCalledWith(filePath); | |
| expect(resolved).toBe(nodeFs.realpathSync.native(filePath)); | |
| } finally { | |
| nativeSpy.mockRestore(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/utils/file-system.test.ts` around lines 101 - 108, The test creates a
spy on nodeFs.realpathSync.native and manually restores it with
nativeSpy.mockRestore, but restoration can be skipped if assertions throw; wrap
the spy usage around the call to FileSystemUtils.canonicalizeExistingPath and
the expect assertions in a try/finally block and call nativeSpy.mockRestore() in
the finally to guarantee cleanup even on failure.
2 participants
Summary
RUNNER~1are expanded before artifact workflow JSON emits file pathsContext
Testing
Summary by CodeRabbit
Refactor
Tests