fix: improve code quality and test coverage (#25)

- Add comprehensive tests for transfer permissions with fetch mocking
- Add workflow timeouts (5-15 min) to prevent hanging jobs
- Add file existence validation in parser before reading

Author: TooAngel

.github/workflows/drift-detection.yml

@@ -6,6 +6,7 @@ on:
 jobs:
   detect-drift:
     runs-on: ubuntu-latest
+    timeout-minutes: 10
     permissions:
       pull-requests: write
       contents: read

.github/workflows/sync-repositories.yml

@@ -8,6 +8,7 @@ on:
 jobs:
   sync:
     runs-on: ubuntu-latest
+    timeout-minutes: 15
     # Only run on the main worlddriven organization, not on forks
     if: github.repository_owner == 'worlddriven'
     permissions:

.github/workflows/test.yml

@@ -13,6 +13,7 @@ on:
 jobs:
   test:
     runs-on: ubuntu-latest
+    timeout-minutes: 5
 
     steps:
       - name: Checkout code

scripts/check-transfer-permissions.test.js

@@ -1,96 +1,213 @@
 #!/usr/bin/env node
 
-import { describe, test } from 'node:test';
+import { describe, test, mock, beforeEach, afterEach } from 'node:test';
 import assert from 'node:assert';
-import { checkTransferPermission } from './check-transfer-permissions.js';
+import {
+  checkTransferPermission,
+  checkMultipleTransferPermissions,
+} from './check-transfer-permissions.js';
 
 describe('checkTransferPermission', () => {
-  test('should throw error if token is missing', async () => {
-    await assert.rejects(
-      async () => await checkTransferPermission(null, 'owner/repo'),
-      { message: 'GitHub token is required' }
-    );
+  describe('input validation', () => {
+    test('should throw error if token is missing', async () => {
+      await assert.rejects(
+        async () => await checkTransferPermission(null, 'owner/repo'),
+        { message: 'GitHub token is required' }
+      );
+    });
+
+    test('should throw error if originRepo is missing', async () => {
+      await assert.rejects(
+        async () => await checkTransferPermission('token', ''),
+        { message: 'Origin repository must be in format "owner/repo-name"' }
+      );
+    });
+
+    test('should throw error if originRepo format is invalid', async () => {
+      await assert.rejects(
+        async () => await checkTransferPermission('token', 'invalid-format'),
+        { message: 'Origin repository must be in format "owner/repo-name"' }
+      );
+    });
+
+    test('should throw error if originRepo has empty owner or repo', async () => {
+      await assert.rejects(
+        async () => await checkTransferPermission('token', '/repo'),
+        { message: 'Invalid origin repository format' }
+      );
+
+      await assert.rejects(
+        async () => await checkTransferPermission('token', 'owner/'),
+        { message: 'Invalid origin repository format' }
+      );
+    });
   });
 
-  test('should throw error if originRepo is missing', async () => {
-    await assert.rejects(
-      async () => await checkTransferPermission('token', ''),
-      { message: 'Origin repository must be in format "owner/repo-name"' }
-    );
+  describe('API response handling', () => {
+    let originalFetch;
+
+    beforeEach(() => {
+      originalFetch = globalThis.fetch;
+    });
+
+    afterEach(() => {
+      globalThis.fetch = originalFetch;
+    });
+
+    test('should return hasPermission true when user has admin access', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () =>
+            Promise.resolve({
+              permissions: { admin: true, push: true, pull: true },
+            }),
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, true);
+      assert.strictEqual(result.permissionLevel, 'admin');
+      assert.ok(result.details.includes('admin access'));
+    });
+
+    test('should return hasPermission false when user has write access only', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () =>
+            Promise.resolve({
+              permissions: { admin: false, push: true, pull: true },
+            }),
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'write');
+      assert.ok(result.details.includes('admin required'));
+    });
+
+    test('should return hasPermission false when user has read access only', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () =>
+            Promise.resolve({
+              permissions: { admin: false, push: false, pull: true },
+            }),
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'read');
+    });
+
+    test('should handle 404 response for non-existent repository', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: false,
+          status: 404,
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'none');
+      assert.ok(result.details.includes('not found'));
+    });
+
+    test('should handle API errors gracefully', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: false,
+          status: 403,
+          text: () => Promise.resolve('Rate limit exceeded'),
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'unknown');
+      assert.ok(result.details.includes('403'));
+    });
+
+    test('should handle network errors gracefully', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.reject(new Error('Network error'))
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'error');
+      assert.ok(result.details.includes('Network error'));
+    });
+
+    test('should handle missing permissions object in response', async () => {
+      globalThis.fetch = mock.fn(() =>
+        Promise.resolve({
+          ok: true,
+          status: 200,
+          json: () => Promise.resolve({}),
+        })
+      );
+
+      const result = await checkTransferPermission('test-token', 'owner/repo');
+
+      assert.strictEqual(result.hasPermission, false);
+      assert.strictEqual(result.permissionLevel, 'none');
+    });
+  });
+});
+
+describe('checkMultipleTransferPermissions', () => {
+  let originalFetch;
+
+  beforeEach(() => {
+    originalFetch = globalThis.fetch;
   });
 
-  test('should throw error if originRepo format is invalid', async () => {
-    await assert.rejects(
-      async () => await checkTransferPermission('token', 'invalid-format'),
-      { message: 'Origin repository must be in format "owner/repo-name"' }
-    );
+  afterEach(() => {
+    globalThis.fetch = originalFetch;
   });
 
-  test('should throw error if originRepo has empty owner or repo', async () => {
-    await assert.rejects(
-      async () => await checkTransferPermission('token', '/repo'),
-      { message: 'Invalid origin repository format' }
-    );
+  test('should return a Map with results for all repositories', async () => {
+    let callCount = 0;
+    globalThis.fetch = mock.fn(() => {
+      callCount++;
+      return Promise.resolve({
+        ok: true,
+        status: 200,
+        json: () =>
+          Promise.resolve({
+            permissions: { admin: callCount === 1, push: true, pull: true },
+          }),
+      });
+    });
 
-    await assert.rejects(
-      async () => await checkTransferPermission('token', 'owner/'),
-      { message: 'Invalid origin repository format' }
-    );
+    const repos = ['owner/repo1', 'owner/repo2'];
+    const results = await checkMultipleTransferPermissions('test-token', repos);
+
+    assert.ok(results instanceof Map);
+    assert.strictEqual(results.size, 2);
+    assert.strictEqual(results.get('owner/repo1').hasPermission, true);
+    assert.strictEqual(results.get('owner/repo2').hasPermission, false);
   });
 
-  // Note: The following tests would require mocking the fetch API
-  // or using a test GitHub token with known repositories.
-  // For now, we document the expected behavior:
-
-  /**
-   * Test case for admin permission (success scenario):
-   * - Repository exists
-   * - worlddriven has admin access
-   * - Expected result:
-   *   {
-   *     hasPermission: true,
-   *     permissionLevel: 'admin',
-   *     details: '✅ worlddriven has admin access to owner/repo'
-   *   }
-   */
-
-  /**
-   * Test case for write permission (insufficient):
-   * - Repository exists
-   * - worlddriven has write (but not admin) access
-   * - Expected result:
-   *   {
-   *     hasPermission: false,
-   *     permissionLevel: 'write',
-   *     details: '❌ worlddriven has "write" access to owner/repo (admin required)'
-   *   }
-   */
-
-  /**
-   * Test case for non-existent repository:
-   * - Repository doesn't exist or worlddriven has no access
-   * - API returns 404
-   * - Expected result:
-   *   {
-   *     hasPermission: false,
-   *     permissionLevel: 'none',
-   *     details: 'Repository owner/repo not found or worlddriven has no access'
-   *   }
-   */
-
-  /**
-   * Test case for API errors:
-   * - Network errors, rate limits, etc.
-   * - Expected result:
-   *   {
-   *     hasPermission: false,
-   *     permissionLevel: 'error' or 'unknown',
-   *     details: 'Error checking permissions: ...'
-   *   }
-   */
-});
+  test('should return empty Map for empty input array', async () => {
+    const results = await checkMultipleTransferPermissions('test-token', []);
 
-// To run integration tests with actual GitHub API:
-// 1. Set WORLDDRIVEN_GITHUB_TOKEN environment variable
-// 2. Create test repositories with known permission levels
-// 3. Run: node --test scripts/check-transfer-permissions.test.js
+    assert.ok(results instanceof Map);
+    assert.strictEqual(results.size, 0);
+  });
+});

scripts/parse-repositories.js

@@ -5,7 +5,8 @@
  * Returns an array of repository objects
  */
 
-import { readFile } from 'fs/promises';
+import { readFile, access } from 'fs/promises';
+import { constants } from 'fs';
 import { fileURLToPath } from 'url';
 import { dirname, join } from 'path';
 
@@ -102,6 +103,14 @@ function parseRepositories(content) {
 async function parseRepositoriesFile() {
   const repoFilePath = join(__dirname, '..', 'REPOSITORIES.md');
 
+  // Validate file exists before attempting to read
+  try {
+    await access(repoFilePath, constants.R_OK);
+  } catch {
+    console.error(`Error: REPOSITORIES.md not found at ${repoFilePath}`);
+    process.exit(1);
+  }
+
   try {
     const content = await readFile(repoFilePath, 'utf-8');
     return parseRepositories(content);