Address review feedback: deterministic tests, cached normalization, simplified promisePool (#9)
* Initial plan * Apply PR review comments: deterministic delays, cached normalization, simplified promisePool Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Improve comment clarity for ecoLower field 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>
This commit is contained in:
Copilot
@@ -747,8 +747,8 @@ test('addChangeVulnerabilitiesToSummary() - respects concurrency limit for API c
|
||||
currentConcurrent++
|
||||
maxConcurrent = Math.max(maxConcurrent, currentConcurrent)
|
||||
|
||||
// Simulate async API call with variable delay
|
||||
await new Promise(resolve => setTimeout(resolve, Math.random() * 10))
|
||||
// Simulate async API call with a small deterministic delay
|
||||
await new Promise(resolve => setTimeout(resolve, 5))
|
||||
|
||||
currentConcurrent--
|
||||
|
||||
|
||||
+15
-15
@@ -1820,15 +1820,11 @@ function countScorecardWarnings(scorecard, config) {
|
||||
*/
|
||||
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
|
||||
for (const task of tasks) {
|
||||
// Execute task and clean up
|
||||
const wrappedPromise = (() => __awaiter(this, void 0, void 0, function* () {
|
||||
const result = yield task();
|
||||
results[index] = result;
|
||||
yield task();
|
||||
}))();
|
||||
executing.add(wrappedPromise);
|
||||
// When promise completes, remove it from the executing set
|
||||
@@ -1842,7 +1838,6 @@ function promisePool(tasks, limit) {
|
||||
}
|
||||
// Wait for all remaining promises
|
||||
yield Promise.all(executing);
|
||||
return results;
|
||||
});
|
||||
}
|
||||
function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
|
||||
@@ -1860,6 +1855,7 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
|
||||
}
|
||||
// Query GitHub API for patch info with concurrency limiting
|
||||
// Store all vulnerability entries (may be multiple per package with different ranges)
|
||||
// Pre-normalize ecosystem, package name, and range to avoid repeated work in rendering
|
||||
const patchInfo = {};
|
||||
const apiClient = (0, utils_1.octokitClient)();
|
||||
// Create tasks for promise pool
|
||||
@@ -1879,11 +1875,17 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
|
||||
const vulnRange = v.vulnerable_version_range || '';
|
||||
const patchVerId = extractPatchVersionId(v.first_patched_version);
|
||||
if (patchVerId) {
|
||||
// Pre-normalize and cache values to avoid repeated work in rendering loop
|
||||
const trimmedRange = vulnRange.trim();
|
||||
const normalizedRange = trimmedRange.replace(/,\s*/g, ' ');
|
||||
patchInfo[advId].push({
|
||||
eco: normalizedEco,
|
||||
pkg: pkgName,
|
||||
range: vulnRange,
|
||||
patch: patchVerId
|
||||
patch: patchVerId,
|
||||
ecoLower: normalizedEco, // Ecosystem already normalized to lowercase
|
||||
pkgLower: pkgName.toLowerCase(),
|
||||
normalizedRange
|
||||
});
|
||||
core.debug(`Added patch info for ${pkgName} (${normalizedEco}): ${patchVerId} for range ${vulnRange}`);
|
||||
}
|
||||
@@ -1920,19 +1922,17 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
|
||||
const normalizedChangeVersion = change.version.trim();
|
||||
core.debug(`Looking up patch for ${change.name}@${change.version} (${ecoLowercase}) in ${vuln.advisory_ghsa_id}`);
|
||||
// Find matching entry by ecosystem, package name (case-insensitive), and version range
|
||||
// Use pre-normalized values from cache to avoid repeated lowercasing and range conversion
|
||||
let foundEntry;
|
||||
for (const vulnEntry of advisoryEntries) {
|
||||
if (vulnEntry.eco.toLowerCase() !== ecoLowercase)
|
||||
if (vulnEntry.ecoLower !== ecoLowercase)
|
||||
continue;
|
||||
if (vulnEntry.pkg.toLowerCase() !== packageLowercase)
|
||||
if (vulnEntry.pkgLower !== packageLowercase)
|
||||
continue;
|
||||
// Normalize range once for both cache key and function call
|
||||
const trimmedRangeOnce = vulnEntry.range.trim();
|
||||
const normalizedRange = trimmedRangeOnce.replace(/,\s*/g, ' ');
|
||||
// Use fail-open (failClosed: false) for patch selection to avoid
|
||||
// incorrectly matching on invalid ranges
|
||||
// Use preTrimmed and preNormalized optimizations since we've done both
|
||||
const isInRange = versionInRange(normalizedChangeVersion, normalizedRange, { preTrimmed: true, preNormalized: true, failClosed: false });
|
||||
const isInRange = versionInRange(normalizedChangeVersion, vulnEntry.normalizedRange, { preTrimmed: true, preNormalized: true, failClosed: false });
|
||||
if (isInRange) {
|
||||
foundEntry = vulnEntry;
|
||||
break;
|
||||
|
||||
+1
-1
File diff suppressed because one or more lines are too long
+27
-21
@@ -246,21 +246,16 @@ function countScorecardWarnings(
|
||||
* @param tasks - Array of functions that return promises
|
||||
* @param limit - Maximum number of concurrent promises
|
||||
*/
|
||||
async function promisePool<T>(
|
||||
tasks: (() => Promise<T>)[],
|
||||
async function promisePool(
|
||||
tasks: (() => Promise<void>)[],
|
||||
limit: number
|
||||
): Promise<T[]> {
|
||||
const results: T[] = []
|
||||
): Promise<void> {
|
||||
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
|
||||
for (const task of tasks) {
|
||||
// Execute task and clean up
|
||||
const wrappedPromise = (async () => {
|
||||
const result = await task()
|
||||
results[index] = result
|
||||
await task()
|
||||
})()
|
||||
|
||||
executing.add(wrappedPromise)
|
||||
@@ -278,7 +273,6 @@ async function promisePool<T>(
|
||||
|
||||
// Wait for all remaining promises
|
||||
await Promise.all(executing)
|
||||
return results
|
||||
}
|
||||
|
||||
export async function addChangeVulnerabilitiesToSummary(
|
||||
@@ -301,9 +295,18 @@ export async function addChangeVulnerabilitiesToSummary(
|
||||
|
||||
// Query GitHub API for patch info with concurrency limiting
|
||||
// Store all vulnerability entries (may be multiple per package with different ranges)
|
||||
// Pre-normalize ecosystem, package name, and range to avoid repeated work in rendering
|
||||
const patchInfo: Record<
|
||||
string,
|
||||
{eco: string; pkg: string; range: string; patch: string}[]
|
||||
{
|
||||
eco: string
|
||||
pkg: string
|
||||
range: string
|
||||
patch: string
|
||||
ecoLower: string
|
||||
pkgLower: string
|
||||
normalizedRange: string
|
||||
}[]
|
||||
> = {}
|
||||
const apiClient = octokitClient()
|
||||
|
||||
@@ -326,11 +329,17 @@ export async function addChangeVulnerabilitiesToSummary(
|
||||
const vulnRange = v.vulnerable_version_range || ''
|
||||
const patchVerId = extractPatchVersionId(v.first_patched_version)
|
||||
if (patchVerId) {
|
||||
// Pre-normalize and cache values to avoid repeated work in rendering loop
|
||||
const trimmedRange = vulnRange.trim()
|
||||
const normalizedRange = trimmedRange.replace(/,\s*/g, ' ')
|
||||
patchInfo[advId].push({
|
||||
eco: normalizedEco,
|
||||
pkg: pkgName,
|
||||
range: vulnRange,
|
||||
patch: patchVerId
|
||||
patch: patchVerId,
|
||||
ecoLower: normalizedEco, // Ecosystem already normalized to lowercase
|
||||
pkgLower: pkgName.toLowerCase(),
|
||||
normalizedRange
|
||||
})
|
||||
core.debug(
|
||||
`Added patch info for ${pkgName} (${normalizedEco}): ${patchVerId} for range ${vulnRange}`
|
||||
@@ -380,23 +389,20 @@ export async function addChangeVulnerabilitiesToSummary(
|
||||
)
|
||||
|
||||
// Find matching entry by ecosystem, package name (case-insensitive), and version range
|
||||
// Use pre-normalized values from cache to avoid repeated lowercasing and range conversion
|
||||
let foundEntry:
|
||||
| {eco: string; pkg: string; range: string; patch: string}
|
||||
| undefined
|
||||
for (const vulnEntry of advisoryEntries) {
|
||||
if (vulnEntry.eco.toLowerCase() !== ecoLowercase) continue
|
||||
if (vulnEntry.pkg.toLowerCase() !== packageLowercase) continue
|
||||
|
||||
// Normalize range once for both cache key and function call
|
||||
const trimmedRangeOnce = vulnEntry.range.trim()
|
||||
const normalizedRange = trimmedRangeOnce.replace(/,\s*/g, ' ')
|
||||
if (vulnEntry.ecoLower !== ecoLowercase) continue
|
||||
if (vulnEntry.pkgLower !== packageLowercase) continue
|
||||
|
||||
// Use fail-open (failClosed: false) for patch selection to avoid
|
||||
// incorrectly matching on invalid ranges
|
||||
// Use preTrimmed and preNormalized optimizations since we've done both
|
||||
const isInRange = versionInRange(
|
||||
normalizedChangeVersion,
|
||||
normalizedRange,
|
||||
vulnEntry.normalizedRange,
|
||||
{preTrimmed: true, preNormalized: true, failClosed: false}
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user