Implement review feedback: concurrency limiting, semver coercion, logging improvements, and test coverage by Copilot · Pull Request #8 · forks-felickz/dependency-review-action

Copilot · 2026-02-09T01:45:49Z

Purpose

Implements all feedback from Copilot review #3770382315 on PR actions#1045 to address API rate limiting risk, version validation gaps, and test hygiene issues.

Key changes:

API concurrency limiting: Promise pool caps concurrent advisory API calls at 10 to prevent secondary rate limits on large PRs
Semver coercion fallback: semver.coerce() in fail-open mode handles real-world version formats (8.0, date-based, 4-part) that aren't strict semver but are comparable. Fail-closed mode remains strict.
Debug log density: Limit advisory entry logs to 5 entries with eco:pkg range -> patch format instead of full JSON dump
Test mock cleanup: jest.restoreAllMocks() instead of clearAllMocks() prevents spy stacking across tests
Promise pool simplification: Changed from Map<Promise<void>, number> to Set<Promise<void>> to remove unused index storage and reduce cognitive overhead
Comprehensive test coverage: Added 4 new tests covering semver coercion success/failure cases and concurrency limiting behavior with varying task durations

Example of improved version handling:

// Before: "8.0" would fail and return N/A for patch version
// After: coerced to "8.0.0" in fail-open mode, enables proper range matching
versionInRange("8.0", ">= 8.0.0, < 9.0.0", {failClosed: false})
// Logs: "Coerced version "8.0" to "8.0.0" for range matching"

Test coverage:

✅ Semver coercion succeeds for non-strict versions (e.g., "8.0" → "8.0.0")
✅ Invalid versions that can't be coerced gracefully return N/A
✅ Concurrency limit never exceeds 10 concurrent API requests
✅ All tasks complete successfully with varying durations (prevents serialization/over-parallelization)

Related Issues

Addresses feedback from PR actions#1045 review #3770382315

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…proved logging, test fixes Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Copilot

Pull request overview

Implements prior review feedback to reduce GitHub Advisory API rate-limit risk, improve real-world version range matching, and reduce noisy debugging output while stabilizing Jest mocks.

Changes:

Added a concurrency-limited promise pool for advisory API requests (cap = 10).
Added fail-open semver.coerce() fallback for non-strict version strings.
Reduced debug log verbosity for “no matching patch” cases; switched Jest cleanup to restoreAllMocks().

Reviewed changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/summary.ts	Adds concurrency-limited advisory fetching, semver coercion fallback, and denser debug logs.
tests/summary.test.ts	Updates mock cleanup to `jest.restoreAllMocks()` to prevent mock stacking.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T13:55:24Z

src/summary.ts

+  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`
+      )
+    }
+  }


The new fail-open coercion path changes version matching behavior in a user-visible way (e.g., "8.0" becoming "8.0.0"). Add tests covering at least: (1) coercion succeeds and versionInRange() returns true for an otherwise-invalid version when failClosed: false, (2) fail-closed behavior remains strict (invalid versions cause the fail-closed branch), and (3) coercion failure still yields the expected “not in range” outcome.

Added comprehensive tests for semver coercion in commit 6ff1d10:

Test for successful coercion (e.g., "8.0" → "8.0.0") in fail-open mode

Test for fail-open behavior with invalid versions that can't be coerced

Verified fail-closed mode remains strict (existing tests confirm this)

Copilot · 2026-02-09T13:55:25Z

src/summary.ts

+async function promisePool<T>(
+  tasks: (() => Promise<T>)[],
+  limit: number
+): Promise<T[]> {
+  const results: T[] = []
+  const executing: Map<Promise<void>, number> = new Map()
+
+  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.set(wrappedPromise, index)
+
+    // 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.keys())
+    }
+  }


The new concurrency limiter is critical to the PR’s stated goal (avoiding secondary rate limits), but it’s currently untested. Add unit tests for promisePool() (or for its integration point) to verify the max in-flight task count never exceeds limit, and that all tasks complete even when task durations vary (to prevent regressions that accidentally serialize or over-parallelize requests).

Added unit tests for promisePool concurrency limiting in commit 6ff1d10:

Test verifies max concurrent requests never exceeds limit (10)

Test confirms all tasks complete with varying durations

Both tests use mock API calls with variable delays to simulate real-world conditions

Copilot · 2026-02-09T13:55:25Z

src/summary.ts

+  const results: T[] = []
+  const executing: Map<Promise<void>, number> = new Map()
+
+  for (let i = 0; i < tasks.length; i++) {
+    const task = tasks[i]
+    const index = i


executing is a Map<Promise<void>, number> but the stored index value is never read, which adds noise and suggests missing logic. Consider switching to Set<Promise<void>> (and remove the unused index value storage) to simplify the pool implementation and reduce cognitive overhead.

Simplified to Set<Promise<void>> in commit 6ff1d10. Removed unused index storage.

Copilot · 2026-02-09T13:55:25Z

src/summary.ts

+      results[index] = result
+    })()
+
+    executing.set(wrappedPromise, index)


executing is a Map<Promise<void>, number> but the stored index value is never read, which adds noise and suggests missing logic. Consider switching to Set<Promise<void>> (and remove the unused index value storage) to simplify the pool implementation and reduce cognitive overhead.

Simplified to Set<Promise<void>> in commit 6ff1d10. Removed unused index storage.

felickz · 2026-02-09T14:03:50Z

@copilot apply changes based on the comments in this thread

…Map to Set Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T14:59:43Z

__tests__/summary.test.ts

+  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--


Using Math.random() in tests can introduce flakiness (timing-dependent behavior that varies per run/CI machine). Prefer deterministic delays (e.g., based on call index) or use fake timers to control scheduling deterministically while still exercising concurrency behavior.

Copilot · 2026-02-09T14:59:43Z

__tests__/summary.test.ts

+      // Variable delay to simulate real-world API response times
+      const delay = Math.random() * 50


Same concern as above: randomized delays can make this test intermittently fail or become noisy in CI. Consider using a fixed set of delays (e.g., alternating 0ms/25ms/50ms or based on i) or fake timers to ensure reproducibility.

Suggested change

// Variable delay to simulate real-world API response times

const delay = Math.random() * 50

// Deterministic variable delay to simulate real-world API response times

const idNumber = parseInt(params.ghsa_id.slice(-4), 10)

const delay = (idNumber % 3) * 25

Copilot · 2026-02-09T14:59:44Z

__tests__/summary.test.ts


 afterEach(() => {
-  jest.clearAllMocks()
+  jest.restoreAllMocks()


jest.restoreAllMocks() restores spied-on originals, but it does not clear call history/state for plain jest.fn() mocks the way jest.clearAllMocks() did. If any tests assert call counts/args without reinitializing those mocks in beforeEach, this can cause cross-test leakage. Consider using jest.restoreAllMocks(); jest.clearAllMocks(); (or jest.resetAllMocks() if you also want implementations reset) to keep both “no spy stacking” and per-test isolation.

Suggested change

jest.restoreAllMocks()

jest.restoreAllMocks()

jest.clearAllMocks()

Initial plan

04b8526

Copilot AI assigned Copilot and felickz Feb 9, 2026

Copilot started work on behalf of felickz February 9, 2026 01:46 View session

Copilot AI and others added 2 commits February 9, 2026 01:49

Implement PR review comments: concurrency limiting, semver coerce, im…

e103d8b

…proved logging, test fixes Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Fix promise pool race condition and remove .then() usage

cb3f901

Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Implement review comments from pull request 1045~~ Implement review feedback: concurrency limiting, semver coercion, logging improvements Feb 9, 2026

Copilot AI requested a review from felickz February 9, 2026 01:54

Copilot finished work on behalf of felickz February 9, 2026 01:54

felickz requested a review from Copilot February 9, 2026 13:53

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Copilot started work on behalf of felickz February 9, 2026 14:04 View session

Add tests for semver coercion and promise pool concurrency, simplify …

6ff1d10

…Map to Set Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>

Copilot AI changed the title ~~Implement review feedback: concurrency limiting, semver coercion, logging improvements~~ Implement review feedback: concurrency limiting, semver coercion, logging improvements, and test coverage Feb 9, 2026

Copilot finished work on behalf of felickz February 9, 2026 14:09

felickz requested a review from Copilot February 9, 2026 14:57

Copilot AI reviewed Feb 9, 2026

View reviewed changes

felickz marked this pull request as ready for review February 9, 2026 19:26

felickz merged commit 539c79b into main Feb 9, 2026
7 checks passed

		// Variable delay to simulate real-world API response times
		const delay = Math.random() * 50

-      // Variable delay to simulate real-world API response times
-      const delay = Math.random() * 50
+      // Deterministic variable delay to simulate real-world API response times
+      const idNumber = parseInt(params.ghsa_id.slice(-4), 10)
+      const delay = (idNumber % 3) * 25

	jest.restoreAllMocks()
	jest.restoreAllMocks()
	jest.clearAllMocks()

Conversation

Copilot AI commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Related Issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

felickz commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Feb 9, 2026 •

edited

Loading