fix: skip Windows permission tests that rely on chmod() #464
Conversation
fs.chmod() on directories doesn't restrict write access on Windows since Windows uses ACLs that Node.js doesn't control. Additionally, admin users and CI runners can bypass read-only attributes. Skip these tests on Windows and use platform-specific invalid paths in cross-platform tests. Fixes #401 (bash/pwsh completion commit breaking Windows e2e tests).
📝 WalkthroughWalkthroughThe pull request modifies test files to improve cross-platform compatibility, particularly for Windows environments. Changes include replacing hard-coded paths with platform-aware logic in bash-installer tests and skipping permission-related tests on Windows due to fundamental differences in file permission handling between operating systems. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/utils/file-system.test.ts (1)
199-212: Guard looks right for Windows, but consider also skipping when running as root (POSIX) to avoid flakiness.
chmod-based “read-only directory” assertions can fail if the test runner is elevated (e.g., uid 0 in containers), similar to the Windows admin/ACL issue.Proposed adjustment
- // Skip on Windows: fs.chmod() on directories doesn't restrict write access on Windows - // Windows uses ACLs which Node.js chmod doesn't control - it.skipIf(process.platform === 'win32')('should return false for non-existent file in read-only directory', async () => { + // Skip on Windows: fs.chmod() on directories doesn't restrict write access on Windows (ACLs). + // Also skip when running as root on POSIX since root can typically bypass mode bits. + const isRoot = typeof process.getuid === 'function' && process.getuid() === 0; + it.skipIf(process.platform === 'win32' || isRoot)( + 'should return false for non-existent file in read-only directory', + async () => { const readOnlyDir = path.join(testDir, 'readonly-dir'); await fs.mkdir(readOnlyDir); await fs.chmod(readOnlyDir, 0o555); // Read-only + execute @@ - }); + } + );test/core/completions/installers/powershell-installer.test.ts (1)
240-257: Windows skips make sense; consider also skipping chmod-based permission tests under elevated POSIX (uid 0) to prevent container CI flakes.
Same underlying issue: permission-bit simulations don’t hold under elevated privileges.If you want to keep the assertions, consider adding an
isRootguard similar to the file-system test suggestion (or converting these to deterministic “ENOTDIR” style failures when feasible).Also applies to: 456-475, 503-520, 623-645
test/core/completions/installers/fish-installer.test.ts (2)
195-210: Windows skip is appropriate; consider also skipping when running as root (POSIX) to avoid permission-bit bypass flakes.
This test expects a permission failure fromchmod(0o444)on a directory, which may not hold under elevated runners.
195-210: Add win32 guard to uninstall test's chmod call at line 295.The uninstall test (lines 288-305) calls
fs.chmod(parentDir, 0o444)at line 295 without a Windows skip guard. This chmod on a directory will be unreliable on Windows since ACLs control access, not Unix permissions. Addit.skipIf(process.platform === 'win32')to prevent test flakiness on Windows CI runners.
🤖 Fix all issues with AI agents
In @test/core/completions/installers/bash-installer.test.ts:
- Around line 143-147: The tests use a flaky invalidPath; instead create a
temporary file and pass its path as the BashInstaller homeDir so attempts to
create directories beneath it reliably fail with ENOTDIR on all platforms.
Update the two test sites that construct new BashInstaller(invalidPath) (around
the invalidInstaller and the second occurrence) to: create a temp file (e.g.,
via fs.mkdtempSync+fs.writeFileSync or tmp.fileSync), use that file's path as
the homeDir for new BashInstaller, and ensure the temp file is cleaned up after
the test; this guarantees deterministic ENOTDIR failures instead of using /root
or Z: paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/core/completions/installers/bash-installer.test.tstest/core/completions/installers/fish-installer.test.tstest/core/completions/installers/powershell-installer.test.tstest/utils/file-system.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/core/completions/installers/bash-installer.test.ts (1)
src/core/completions/installers/bash-installer.ts (1)
BashInstaller(11-366)
🔇 Additional comments (2)
test/core/completions/installers/powershell-installer.test.ts (1)
240-257:it.skipIf(...)is supported in Vitest 3.2.4. The repository uses Vitest 3.2.4, and the API is correctly implemented as an alias fortest.skipIf(condition)(name, fn). All usages at lines 240-257, 456-475, 503-520, and 623-645 are valid and follow the documented pattern.test/utils/file-system.test.ts (1)
199-212: No action needed —it.skipIf(...)is supported in Vitest 3.2.4.Vitest v3 (including 3.2.4) exposes
test.skipIf/it.skipIffor conditional test skipping. The syntaxit.skipIf(condition)('name', fn)is the correct API and will work as intended.
| // Use a path that will fail on both Unix and Windows | ||
| const invalidPath = process.platform === 'win32' | ||
| ? 'Z:\\nonexistent\\invalid\\path' // Non-existent drive letter on Windows | ||
| : '/root/invalid/nonexistent/path'; // Permission-denied path on Unix | ||
| const invalidInstaller = new BashInstaller(invalidPath); |
There was a problem hiding this comment.
invalidPath is potentially flaky (/root if tests run as root; Z: if mapped). Prefer a deterministic “ENOTDIR” failure.
Instead of relying on permissions or a non-existent drive, create a file and use it as homeDir so directory creation beneath it fails consistently on all platforms/users.
Proposed fix (apply to both tests)
- // Use a path that will fail on both Unix and Windows
- const invalidPath = process.platform === 'win32'
- ? 'Z:\\nonexistent\\invalid\\path' // Non-existent drive letter on Windows
- : '/root/invalid/nonexistent/path'; // Permission-denied path on Unix
- const invalidInstaller = new BashInstaller(invalidPath);
+ // Deterministic failure on all platforms: homeDir points to a file, not a directory (ENOTDIR).
+ const invalidHomeDir = path.join(os.tmpdir(), `openspec-invalid-home-${randomUUID()}`);
+ await fs.writeFile(invalidHomeDir, 'not-a-directory');
+ const invalidInstaller = new BashInstaller(invalidHomeDir);
- const result = await invalidInstaller.install(testScript);
+ const result = await invalidInstaller.install(testScript);
expect(result.success).toBe(false);
expect(result.message).toContain('Failed to install');
+
+ await fs.rm(invalidHomeDir, { force: true });- // Use a path that will fail on both Unix and Windows
- const invalidPath = process.platform === 'win32'
- ? 'Z:\\nonexistent\\invalid\\path' // Non-existent drive letter on Windows
- : '/root/invalid/path'; // Permission-denied path on Unix
- const invalidInstaller = new BashInstaller(invalidPath);
+ // Deterministic failure on all platforms: homeDir points to a file, not a directory (ENOTDIR).
+ const invalidHomeDir = path.join(os.tmpdir(), `openspec-invalid-home-${randomUUID()}`);
+ await fs.writeFile(invalidHomeDir, 'not-a-directory');
+ const invalidInstaller = new BashInstaller(invalidHomeDir);
@@
const result = await invalidInstaller.configureBashrc(completionsDir);
expect(result).toBe(false);
+
+ await fs.rm(invalidHomeDir, { force: true });Also applies to: 381-385
🤖 Prompt for AI Agents
In @test/core/completions/installers/bash-installer.test.ts around lines 143 -
147, The tests use a flaky invalidPath; instead create a temporary file and pass
its path as the BashInstaller homeDir so attempts to create directories beneath
it reliably fail with ENOTDIR on all platforms. Update the two test sites that
construct new BashInstaller(invalidPath) (around the invalidInstaller and the
second occurrence) to: create a temp file (e.g., via
fs.mkdtempSync+fs.writeFileSync or tmp.fileSync), use that file's path as the
homeDir for new BashInstaller, and ensure the temp file is cleaned up after the
test; this guarantees deterministic ENOTDIR failures instead of using /root or
Z: paths.
Summary
Fixed Windows e2e test failures caused by the bash/pwsh completion commit (#401). The root cause is that
fs.chmod()doesn't restrict write access on Windows since Windows uses ACLs. Tests now skip permission checks on Windows and use platform-specific invalid paths for cross-platform tests.Changes
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.