Implement review feedback: concurrency limiting, semver coercion, logging improvements, and test coverage (#8)

* Initial plan * Implement PR review comments: concurrency limiting, semver coerce, improved logging, test fixes Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Fix promise pool race condition and remove .then() usage Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Add tests for semver coercion and promise pool concurrency, simplify Map to Set Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
2026-02-09 14:28:09 -05:00
parent ee66ea100d
commit 539c79be65
4 changed files with 339 additions and 49 deletions
@@ -19,7 +19,7 @@ beforeEach(() => {
 })

 afterEach(() => {
-  jest.clearAllMocks()
+  jest.restoreAllMocks()
  core.summary.emptyBuffer()
 })

@@ -639,3 +639,184 @@ test('addChangeVulnerabilitiesToSummary() - handles RestSharp GHSA-4rr6-2v9v-wcp
    ghsa_id: 'GHSA-4rr6-2v9v-wcpc'
  })
 })
+
+test('addChangeVulnerabilitiesToSummary() - handles version coercion for non-strict semver versions', async () => {
+  // Test that versions like "8.0" (without patch version) can be coerced to "8.0.0"
+  // for successful range matching in fail-open mode (patch selection)
+  const pkg = createTestChange({
+    ecosystem: 'npm',
+    name: 'test-package',
+    version: '8.0', // Non-strict semver version
+    vulnerabilities: [
+      createTestVulnerability({
+        advisory_ghsa_id: 'GHSA-test-1234',
+        advisory_summary: 'Test vulnerability',
+        severity: 'high'
+      })
+    ]
+  })
+
+  mockOctokitRequest.mockResolvedValueOnce({
+    data: {
+      vulnerabilities: [
+        {
+          package: {
+            ecosystem: 'npm',
+            name: 'test-package'
+          },
+          vulnerable_version_range: '>= 8.0.0, < 9.0.0',
+          first_patched_version: '9.0.0'
+        }
+      ]
+    }
+  })
+
+  const changes = [pkg]
+  await summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+
+  // Should coerce "8.0" to "8.0.0" and successfully match the range,
+  // showing the patched version instead of N/A
+  expect(text).toContain('9.0.0')
+  expect(text).not.toContain('N/A')
+})
+
+test('addChangeVulnerabilitiesToSummary() - handles invalid versions in fail-open mode', async () => {
+  // Test that completely invalid versions that can't be coerced
+  // still return N/A gracefully in fail-open mode
+  const pkg = createTestChange({
+    ecosystem: 'npm',
+    name: 'test-package',
+    version: 'invalid-version-string',
+    vulnerabilities: [
+      createTestVulnerability({
+        advisory_ghsa_id: 'GHSA-test-5678',
+        advisory_summary: 'Test vulnerability',
+        severity: 'high'
+      })
+    ]
+  })
+
+  mockOctokitRequest.mockResolvedValueOnce({
+    data: {
+      vulnerabilities: [
+        {
+          package: {
+            ecosystem: 'npm',
+            name: 'test-package'
+          },
+          vulnerable_version_range: '>= 1.0.0, < 2.0.0',
+          first_patched_version: '2.0.0'
+        }
+      ]
+    }
+  })
+
+  const changes = [pkg]
+  await summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+
+  // Should show N/A since version can't be coerced or matched
+  expect(text).toContain('N/A')
+})
+
+test('addChangeVulnerabilitiesToSummary() - respects concurrency limit for API calls', async () => {
+  // Create 15 packages with different vulnerabilities to test concurrency limiting
+  const packages = Array.from({length: 15}, (_, i) =>
+    createTestChange({
+      ecosystem: 'npm',
+      name: `package-${i}`,
+      version: '1.0.0',
+      vulnerabilities: [
+        createTestVulnerability({
+          advisory_ghsa_id: `GHSA-test-${i.toString().padStart(4, '0')}`,
+          advisory_summary: `Vulnerability ${i}`,
+          severity: 'high'
+        })
+      ]
+    })
+  )
+
+  // Track concurrent calls
+  let maxConcurrent = 0
+  let currentConcurrent = 0
+
+  mockOctokitRequest.mockImplementation(async () => {
+    currentConcurrent++
+    maxConcurrent = Math.max(maxConcurrent, currentConcurrent)
+
+    // Simulate async API call with variable delay
+    await new Promise(resolve => setTimeout(resolve, Math.random() * 10))
+
+    currentConcurrent--
+
+    return {
+      data: {
+        vulnerabilities: [
+          {
+            package: {ecosystem: 'npm', name: 'test'},
+            vulnerable_version_range: '>= 1.0.0, < 2.0.0',
+            first_patched_version: '2.0.0'
+          }
+        ]
+      }
+    }
+  })
+
+  await summary.addChangeVulnerabilitiesToSummary(packages, 'low')
+
+  // Verify that concurrency limit (10) was respected
+  expect(maxConcurrent).toBeLessThanOrEqual(10)
+  // Verify all 15 unique advisories were fetched
+  expect(mockOctokitRequest).toHaveBeenCalledTimes(15)
+})
+
+test('addChangeVulnerabilitiesToSummary() - completes all tasks even with varying durations', async () => {
+  // Test that promise pool doesn't lose tasks when some complete faster than others
+  const packages = Array.from({length: 20}, (_, i) =>
+    createTestChange({
+      ecosystem: 'npm',
+      name: `package-${i}`,
+      version: '1.0.0',
+      vulnerabilities: [
+        createTestVulnerability({
+          advisory_ghsa_id: `GHSA-vary-${i.toString().padStart(4, '0')}`,
+          advisory_summary: `Vulnerability ${i}`,
+          severity: 'high'
+        })
+      ]
+    })
+  )
+
+  const completedAdvisories = new Set<string>()
+
+  mockOctokitRequest.mockImplementation(
+    async (path: string, params: {ghsa_id: string}) => {
+      // Variable delay to simulate real-world API response times
+      const delay = Math.random() * 50
+      await new Promise(resolve => setTimeout(resolve, delay))
+
+      completedAdvisories.add(params.ghsa_id)
+
+      return {
+        data: {
+          vulnerabilities: [
+            {
+              package: {ecosystem: 'npm', name: 'test'},
+              vulnerable_version_range: '>= 1.0.0, < 2.0.0',
+              first_patched_version: '2.0.0'
+            }
+          ]
+        }
+      }
+    }
+  )
+
+  await summary.addChangeVulnerabilitiesToSummary(packages, 'low')
+
+  // Verify all 20 unique advisories were fetched and completed
+  expect(completedAdvisories.size).toBe(20)
+  expect(mockOctokitRequest).toHaveBeenCalledTimes(20)
+})
@@ -1654,6 +1654,7 @@ const icons = {
    warning: '⚠️'
 };
 const MAX_SCANNED_FILES_BYTES = 1048576;
+const API_CONCURRENCY_LIMIT = 10; // Limit concurrent API requests to avoid rate limiting
 /**
 * Helper to check if a version falls within a vulnerable range.
 * Uses the `semver` library for proper prerelease handling and range parsing.
@@ -1691,8 +1692,17 @@ function versionInRange(version, range, options = {}) {
        : trimmedRange.replace(/,\s*/g, ' ');
    // Validate version and range explicitly to enforce fail-closed semantics
    // semver.satisfies() typically returns false for invalid inputs without throwing
-    const validVersion = semver.valid(trimmedVersion);
+    let validVersion = semver.valid(trimmedVersion);
    const validRange = semver.validRange(semverRange);
+    // For fail-open mode (patch selection), try coercing invalid versions
+    // to handle common real-world formats like "8.0", date-based versions, etc.
+    if (!validVersion && !failClosed) {
+        const coerced = semver.coerce(trimmedVersion);
+        if (coerced) {
+            validVersion = coerced.version;
+            core.debug(`Coerced version "${trimmedVersion}" to "${validVersion}" for range matching`);
+        }
+    }
    if (!validVersion || !validRange) {
        if (failClosed) {
            const issues = [];
@@ -1803,6 +1813,38 @@ function countScorecardWarnings(scorecard, config) {
                : 0);
    }, 0);
 }
+/**
+ * Execute promises with a concurrency limit to avoid overwhelming APIs.
+ * @param tasks - Array of functions that return promises
+ * @param limit - Maximum number of concurrent promises
+ */
+function promisePool(tasks, limit) {
+    return __awaiter(this, void 0, void 0, function* () {
+        const results = [];
+        const executing = new Set();
+        for (let i = 0; i < tasks.length; i++) {
+            const task = tasks[i];
+            const index = i;
+            // Wrap task execution to store result and clean up
+            const wrappedPromise = (() => __awaiter(this, void 0, void 0, function* () {
+                const result = yield task();
+                results[index] = result;
+            }))();
+            executing.add(wrappedPromise);
+            // When promise completes, remove it from the executing set
+            wrappedPromise.finally(() => {
+                executing.delete(wrappedPromise);
+            });
+            // Wait if we've hit the concurrency limit
+            if (executing.size >= limit) {
+                yield Promise.race(executing);
+            }
+        }
+        // Wait for all remaining promises
+        yield Promise.all(executing);
+        return results;
+    });
+}
 function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
    return __awaiter(this, void 0, void 0, function* () {
        if (vulnerableChanges.length === 0) {
@@ -1816,11 +1858,12 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
                advisorySet.add(vuln.advisory_ghsa_id);
            }
        }
-        // Query GitHub API for patch info in parallel
+        // Query GitHub API for patch info with concurrency limiting
        // Store all vulnerability entries (may be multiple per package with different ranges)
        const patchInfo = {};
        const apiClient = (0, utils_1.octokitClient)();
-        yield Promise.all(Array.from(advisorySet).map((advId) => __awaiter(this, void 0, void 0, function* () {
+        // Create tasks for promise pool
+        const tasks = Array.from(advisorySet).map(advId => () => __awaiter(this, void 0, void 0, function* () {
            try {
                core.debug(`Fetching advisory data for ${advId}`);
                const apiResult = yield apiClient.request('GET /advisories/{ghsa_id}', {
@@ -1855,7 +1898,9 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
                core.debug(`API call failed for ${advId}: ${errorMessage}`);
                patchInfo[advId] = [];
            }
-        })));
+        }));
+        // Execute API calls with concurrency limit
+        yield promisePool(tasks, API_CONCURRENCY_LIMIT);
        core.summary.addHeading('Vulnerabilities', 2);
        for (const manifest of manifests) {
            // Create fresh rows array for each manifest to avoid accumulation
@@ -1898,7 +1943,11 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
                            core.debug(`Found patch version ${patchVer} for ${change.name}@${change.version}`);
                        }
                        else {
-                            core.debug(`No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advisoryEntries)}`);
+                            const maxLoggedEntries = 5;
+                            const entriesPreview = advisoryEntries
+                                .slice(0, maxLoggedEntries)
+                                .map(entry => `${entry.eco}:${entry.pkg} ${entry.range} -> ${entry.patch}`);
+                            core.debug(`No matching patch found for ${change.name}@${change.version}. Available entries (showing up to ${Math.min(advisoryEntries.length, maxLoggedEntries)} of ${advisoryEntries.length}): ${entriesPreview.join('; ')}`);
                        }
                    }
                    else {
@@ -17,6 +17,7 @@ const icons = {
 }

 const MAX_SCANNED_FILES_BYTES = 1048576
+const API_CONCURRENCY_LIMIT = 10 // Limit concurrent API requests to avoid rate limiting

 /**
 * Helper to check if a version falls within a vulnerable range.
@@ -71,9 +72,21 @@ function versionInRange(

  // Validate version and range explicitly to enforce fail-closed semantics
  // semver.satisfies() typically returns false for invalid inputs without throwing
-  const validVersion = semver.valid(trimmedVersion)
+  let validVersion = semver.valid(trimmedVersion)
  const validRange = semver.validRange(semverRange)

+  // For fail-open mode (patch selection), try coercing invalid versions
+  // to handle common real-world formats like "8.0", date-based versions, etc.
+  if (!validVersion && !failClosed) {
+    const coerced = semver.coerce(trimmedVersion)
+    if (coerced) {
+      validVersion = coerced.version
+      core.debug(
+        `Coerced version "${trimmedVersion}" to "${validVersion}" for range matching`
+      )
+    }
+  }
+
  if (!validVersion || !validRange) {
    if (failClosed) {
      const issues: string[] = []
@@ -228,6 +241,46 @@ function countScorecardWarnings(
  )
 }

+/**
+ * Execute promises with a concurrency limit to avoid overwhelming APIs.
+ * @param tasks - Array of functions that return promises
+ * @param limit - Maximum number of concurrent promises
+ */
+async function promisePool<T>(
+  tasks: (() => Promise<T>)[],
+  limit: number
+): Promise<T[]> {
+  const results: T[] = []
+  const executing: Set<Promise<void>> = new Set()
+
+  for (let i = 0; i < tasks.length; i++) {
+    const task = tasks[i]
+    const index = i
+
+    // Wrap task execution to store result and clean up
+    const wrappedPromise = (async () => {
+      const result = await task()
+      results[index] = result
+    })()
+
+    executing.add(wrappedPromise)
+
+    // When promise completes, remove it from the executing set
+    wrappedPromise.finally(() => {
+      executing.delete(wrappedPromise)
+    })
+
+    // Wait if we've hit the concurrency limit
+    if (executing.size >= limit) {
+      await Promise.race(executing)
+    }
+  }
+
+  // Wait for all remaining promises
+  await Promise.all(executing)
+  return results
+}
+
 export async function addChangeVulnerabilitiesToSummary(
  vulnerableChanges: Changes,
  severity: string
@@ -246,7 +299,7 @@ export async function addChangeVulnerabilitiesToSummary(
    }
  }

-  // Query GitHub API for patch info in parallel
+  // Query GitHub API for patch info with concurrency limiting
  // Store all vulnerability entries (may be multiple per package with different ranges)
  const patchInfo: Record<
    string,
@@ -254,50 +307,50 @@ export async function addChangeVulnerabilitiesToSummary(
  > = {}
  const apiClient = octokitClient()

-  await Promise.all(
-    Array.from(advisorySet).map(async advId => {
-      try {
-        core.debug(`Fetching advisory data for ${advId}`)
-        const apiResult = await apiClient.request('GET /advisories/{ghsa_id}', {
-          ghsa_id: advId
-        })
+  // Create tasks for promise pool
+  const tasks = Array.from(advisorySet).map(advId => async () => {
+    try {
+      core.debug(`Fetching advisory data for ${advId}`)
+      const apiResult = await apiClient.request('GET /advisories/{ghsa_id}', {
+        ghsa_id: advId
+      })

-        patchInfo[advId] = []
-        const vulnList = apiResult.data.vulnerabilities || []
-        core.debug(
-          `Found ${vulnList.length} vulnerability entries for ${advId}`
-        )
+      patchInfo[advId] = []
+      const vulnList = apiResult.data.vulnerabilities || []
+      core.debug(`Found ${vulnList.length} vulnerability entries for ${advId}`)

-        for (const v of vulnList) {
-          if (v.package && v.package.ecosystem) {
-            const normalizedEco = v.package.ecosystem.toLowerCase()
-            const pkgName = v.package.name || ''
-            const vulnRange = v.vulnerable_version_range || ''
-            const patchVerId = extractPatchVersionId(v.first_patched_version)
-            if (patchVerId) {
-              patchInfo[advId].push({
-                eco: normalizedEco,
-                pkg: pkgName,
-                range: vulnRange,
-                patch: patchVerId
-              })
-              core.debug(
-                `Added patch info for ${pkgName} (${normalizedEco}): ${patchVerId} for range ${vulnRange}`
-              )
-            } else {
-              core.debug(
-                `No patch version found for ${pkgName} (${normalizedEco}) in ${advId}`
-              )
-            }
+      for (const v of vulnList) {
+        if (v.package && v.package.ecosystem) {
+          const normalizedEco = v.package.ecosystem.toLowerCase()
+          const pkgName = v.package.name || ''
+          const vulnRange = v.vulnerable_version_range || ''
+          const patchVerId = extractPatchVersionId(v.first_patched_version)
+          if (patchVerId) {
+            patchInfo[advId].push({
+              eco: normalizedEco,
+              pkg: pkgName,
+              range: vulnRange,
+              patch: patchVerId
+            })
+            core.debug(
+              `Added patch info for ${pkgName} (${normalizedEco}): ${patchVerId} for range ${vulnRange}`
+            )
+          } else {
+            core.debug(
+              `No patch version found for ${pkgName} (${normalizedEco}) in ${advId}`
+            )
          }
        }
-      } catch (e) {
-        const errorMessage = e instanceof Error ? e.message : String(e)
-        core.debug(`API call failed for ${advId}: ${errorMessage}`)
-        patchInfo[advId] = []
      }
-    })
-  )
+    } catch (e) {
+      const errorMessage = e instanceof Error ? e.message : String(e)
+      core.debug(`API call failed for ${advId}: ${errorMessage}`)
+      patchInfo[advId] = []
+    }
+  })
+
+  // Execute API calls with concurrency limit
+  await promisePool(tasks, API_CONCURRENCY_LIMIT)

  core.summary.addHeading('Vulnerabilities', 2)

@@ -359,8 +412,15 @@ export async function addChangeVulnerabilitiesToSummary(
              `Found patch version ${patchVer} for ${change.name}@${change.version}`
            )
          } else {
+            const maxLoggedEntries = 5
+            const entriesPreview = advisoryEntries
+              .slice(0, maxLoggedEntries)
+              .map(
+                entry =>
+                  `${entry.eco}:${entry.pkg} ${entry.range} -> ${entry.patch}`
+              )
            core.debug(
-              `No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advisoryEntries)}`
+              `No matching patch found for ${change.name}@${change.version}. Available entries (showing up to ${Math.min(advisoryEntries.length, maxLoggedEntries)} of ${advisoryEntries.length}): ${entriesPreview.join('; ')}`
            )
          }
        } else {