Skip to content

fix: skip Windows permission tests that rely on chmod() #464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TabishB merged 1 commit into from
Jan 10, 2026
Merged

fix: skip Windows permission tests that rely on chmod() #464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions test/core/completions/installers/bash-installer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,11 @@ describe('BashInstaller', () => {

it('should handle installation errors gracefully', async () => {
// Create installer with non-existent/invalid home directory
const invalidInstaller = new BashInstaller('/root/invalid/nonexistent/path');
// 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);
Comment on lines +143 to +147
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


const result = await invalidInstaller.install(testScript);

Expand Down Expand Up @@ -374,7 +378,11 @@ describe('BashInstaller', () => {

it('should handle write permission errors gracefully', async () => {
// Create installer with path that can't be written
const invalidInstaller = new BashInstaller('/root/invalid/path');
// 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);

const result = await invalidInstaller.configureBashrc(completionsDir);

Expand Down
4 changes: 3 additions & 1 deletion test/core/completions/installers/fish-installer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,9 @@ complete -c openspec -a 'validate' -d 'Validate specs'
await fs.rm(spacedHomeDir, { recursive: true, force: true });
});

it('should return failure on permission error', async () => {
// 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 failure on permission error', async () => {
// Create a read-only directory to simulate permission error
const restrictedDir = path.join(testHomeDir, '.config', 'fish', 'completions');
await fs.mkdir(restrictedDir, { recursive: true });
Expand Down
16 changes: 12 additions & 4 deletions test/core/completions/installers/powershell-installer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ describe('PowerShellInstaller', () => {
expect(content).toContain('# OPENSPEC:END');
});

it('should return false on write permission error', async () => {
// Skip on Windows: fs.chmod() doesn't reliably restrict write access on Windows
// (admin users can bypass read-only attribute, and CI runners often have elevated privileges)
it.skipIf(process.platform === 'win32')('should return false on write permission error', async () => {
delete process.env.OPENSPEC_NO_AUTO_CONFIG;
const profilePath = installer.getProfilePath();
await fs.mkdir(path.dirname(profilePath), { recursive: true });
Expand Down Expand Up @@ -451,7 +453,9 @@ Register-ArgumentCompleter -CommandName openspec -ScriptBlock $openspecCompleter
// Note: OPENSPEC_NO_AUTO_CONFIG support was removed from PowerShell installer
// Profile is now always auto-configured if possible

it('should provide instructions when profile cannot be configured', async () => {
// Skip on Windows: fs.chmod() doesn't reliably restrict write access on Windows
// (admin users can bypass read-only attribute, and CI runners often have elevated privileges)
it.skipIf(process.platform === 'win32')('should provide instructions when profile cannot be configured', async () => {
delete process.env.OPENSPEC_NO_AUTO_CONFIG;
// Make profile directory read-only to prevent configuration
const profilePath = installer.getProfilePath();
Expand Down Expand Up @@ -496,7 +500,9 @@ Register-ArgumentCompleter -CommandName openspec -ScriptBlock $openspecCompleter
await fs.rm(spacedHomeDir, { recursive: true, force: true });
});

it('should return failure on permission error', async () => {
// 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 failure on permission error', async () => {
const targetPath = installer.getInstallationPath();
const targetDir = path.dirname(targetPath);
await fs.mkdir(targetDir, { recursive: true });
Expand Down Expand Up @@ -614,7 +620,9 @@ Register-ArgumentCompleter -CommandName openspec -ScriptBlock $openspecCompleter
expect(profileContentAfter).not.toContain('# OPENSPEC:START');
});

it('should return failure on permission error', async () => {
// 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 failure on permission error', async () => {
delete process.env.OPENSPEC_NO_AUTO_CONFIG;
await installer.install(mockCompletionScript);
const targetPath = installer.getInstallationPath();
Expand Down
4 changes: 3 additions & 1 deletion test/utils/file-system.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ describe('FileSystemUtils', () => {
expect(canWrite).toBe(true);
});

it('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
// 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 () => {
const readOnlyDir = path.join(testDir, 'readonly-dir');
await fs.mkdir(readOnlyDir);
await fs.chmod(readOnlyDir, 0o555); // Read-only + execute
Expand Down
Loading