Implement review fixes: semver library, scoping, case-insensitive matching, error logging, and configurable fail behavior#7
Conversation
…se-insensitive matching, and rows scoping Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements review-feedback fixes for vulnerable-range matching and summary generation, focusing on correctness and clearer logging.
Changes:
- Replaced custom semver/range parsing with the
semverlibrary and added parse-failure logging. - Fixed per-manifest summary table scoping by resetting
rowsinside the manifest loop. - Normalized package name matching to be case-insensitive and improved error message extraction.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/summary.ts | Uses semver for range checks, fixes rows scoping per manifest, case-insensitive package matching, and improved error message logging. |
| package.json | Adds semver dependency and TypeScript typings entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| core.debug( | ||
| `Failed to parse version range "${range}" for version "${version}": ${error instanceof Error ? error.message : String(error)}` | ||
| ) | ||
| return false |
There was a problem hiding this comment.
The behavior in the catch block conflicts with the comment: returning false means “version is NOT in range”, which is typically fail-open for a function described as checking whether a version falls within a vulnerable range. If the intent is truly “fail closed” for vulnerability evaluation, return true here (treat as in-range/vulnerable), or update the comment/function semantics (and possibly rename) so callers don’t interpret parse failures as “not vulnerable”.
| return false | |
| return true |
There was a problem hiding this comment.
Fixed in e33d3f9. Changed to return true (treat as vulnerable) when range cannot be parsed, implementing proper fail-closed behavior for vulnerability checking.
| "devDependencies": { | ||
| "@types/jest": "^29.5.14", | ||
| "@types/node": "^20", | ||
| "@types/semver": "^7.7.1", |
There was a problem hiding this comment.
semver v7 ships its own TypeScript type definitions, so adding @types/semver is typically redundant and can occasionally cause type conflicts if versions drift. Consider removing @types/semver and relying on the bundled types from semver.
| "@types/semver": "^7.7.1", |
There was a problem hiding this comment.
Fixed in e33d3f9. Removed @types/semver as semver v7 includes built-in TypeScript types.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Supports basic semver comparisons like ">= 8.0.0, <= 8.0.20" | ||
| // Uses semver library for proper prerelease handling and range parsing | ||
| function versionInRange(version: string, range: string): boolean { | ||
| if (!version || !range) return false |
There was a problem hiding this comment.
This early return is fail-open when range is an empty string (it returns false = not vulnerable), which conflicts with the stated fail-closed behavior in the function. Consider treating an empty/unset range as unparseable and returning true (vulnerable), or adjust the comment/contract so the fail-closed policy is applied consistently.
| if (!version || !range) return false | |
| if (!version) return false | |
| if (!range) { | |
| // Fail closed: treat missing/empty range as unparseable and assume vulnerable | |
| core.debug( | |
| `Empty or missing version range for version "${version}". Treating as vulnerable (fail closed).` | |
| ) | |
| return true | |
| } |
There was a problem hiding this comment.
Fixed in d071c8b. Split early return to handle empty range separately - now returns true (vulnerable) with debug message for consistent fail-closed behavior.
| // Convert GitHub API range format to semver format | ||
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | ||
| // Semver expects: ">=8.0.0 <=8.0.20" | ||
| const semverRange = range.replace(/,\s*/g, ' ') | ||
|
|
||
| // Parse the range - semver library handles complex ranges and prereleases | ||
| return semver.satisfies(version, semverRange, {includePrerelease: true}) |
There was a problem hiding this comment.
It’s easy for version/range strings to include leading/trailing whitespace from upstream sources; since semver parsing is strict, this can trigger exceptions and force fail-closed. Trimming version and range (before building semverRange) would make behavior more stable without changing intended semantics.
| // Convert GitHub API range format to semver format | |
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | |
| // Semver expects: ">=8.0.0 <=8.0.20" | |
| const semverRange = range.replace(/,\s*/g, ' ') | |
| // Parse the range - semver library handles complex ranges and prereleases | |
| return semver.satisfies(version, semverRange, {includePrerelease: true}) | |
| const trimmedVersion = version.trim() | |
| const trimmedRange = range.trim() | |
| // Convert GitHub API range format to semver format | |
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | |
| // Semver expects: ">=8.0.0 <=8.0.20" | |
| const semverRange = trimmedRange.replace(/,\s*/g, ' ') | |
| // Parse the range - semver library handles complex ranges and prereleases | |
| return semver.satisfies(trimmedVersion, semverRange, {includePrerelease: true}) |
There was a problem hiding this comment.
Fixed in d071c8b. Added trimming for both version and range strings before processing to handle whitespace from upstream sources and prevent unnecessary parse exceptions.
| const matchingEntry = advData.find( | ||
| entry => | ||
| entry.eco === normalizedEco && | ||
| entry.pkg === change.name && | ||
| entry.pkg.toLowerCase() === pkgNameLower && | ||
| versionInRange(change.version, entry.range) | ||
| ) |
There was a problem hiding this comment.
versionInRange() will parse/construct semver ranges repeatedly inside this .find() (once per entry). If advData can be large, consider memoizing parsed ranges (e.g., a Map<string, boolean | Range> keyed by entry.range) or precomputing a normalized/compiled range representation when building patchInfo to reduce repeated parsing overhead.
There was a problem hiding this comment.
Fixed in d071c8b. Implemented Map-based caching with unique key format to avoid repeated semver parsing. Replaced .find() with explicit loop that checks cache before calling versionInRange().
|
@copilot apply changes based on the comments in this thread |
…ent range check caching Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!version) return false | ||
| if (!range) { | ||
| // Fail closed: treat missing/empty range as unparseable and assume vulnerable | ||
| core.debug( | ||
| `Empty or missing version range for version "${version}". Treating as vulnerable (fail closed).` | ||
| ) | ||
| return true | ||
| } | ||
|
|
||
| return true | ||
| try { | ||
| const trimmedVersion = version.trim() | ||
| const trimmedRange = range.trim() |
There was a problem hiding this comment.
versionInRange() treats missing/empty ranges as fail-closed (true + debug log), but treats missing versions as fail-open (return false) with no debug log. This asymmetry makes behavior harder to reason about, especially when inputs are malformed (e.g., whitespace-only version). Consider aligning the treatment (and logging) of missing/blank version with the fail-closed strategy used for range, or explicitly documenting why missing version is handled differently.
There was a problem hiding this comment.
Fixed in 2728360. Empty/missing version now returns true (vulnerable) with debug logging, aligning with empty range treatment for consistent fail-closed behavior.
| let foundEntry = null | ||
| for (const vulnEntry of advisoryEntries) { | ||
| if (vulnEntry.eco !== ecoLowercase) continue | ||
| if (vulnEntry.pkg.toLowerCase() !== packageLowercase) continue | ||
|
|
||
| const cacheKey = `${change.version}|||${vulnEntry.range}` | ||
| let isInRange = rangeCheckCache.get(cacheKey) | ||
| if (isInRange === undefined) { | ||
| isInRange = versionInRange(change.version, vulnEntry.range) | ||
| rangeCheckCache.set(cacheKey, isInRange) | ||
| } |
There was a problem hiding this comment.
Initializing foundEntry as null can degrade type inference in TypeScript (often widening to any), which can hide real typing issues and reduce editor/compiler help. Prefer a typed undefined (or an explicit union type) for “not found yet” so downstream usage (e.g., foundEntry.patch) stays strongly typed.
There was a problem hiding this comment.
Fixed in 2728360. Changed foundEntry from null to typed undefined with explicit union type to maintain strong TypeScript type inference.
|
|
||
| const cacheKey = `${change.version}|||${vulnEntry.range}` | ||
| let isInRange = rangeCheckCache.get(cacheKey) | ||
| if (isInRange === undefined) { | ||
| isInRange = versionInRange(change.version, vulnEntry.range) | ||
| rangeCheckCache.set(cacheKey, isInRange) | ||
| } | ||
|
|
There was a problem hiding this comment.
The cache key is built from the untrimmed change.version and vulnEntry.range, but versionInRange() trims both before evaluating. If either input includes stray whitespace, you’ll get avoidable cache misses (less effective caching) even though the semver evaluation normalizes them. Consider normalizing (e.g., trimming) the key components before building cacheKey, and avoiding a magic delimiter like ||| by using a safer keying approach (e.g., JSON.stringify([normalizedVersion, normalizedRange]) or nested Maps).
| const cacheKey = `${change.version}|||${vulnEntry.range}` | |
| let isInRange = rangeCheckCache.get(cacheKey) | |
| if (isInRange === undefined) { | |
| isInRange = versionInRange(change.version, vulnEntry.range) | |
| rangeCheckCache.set(cacheKey, isInRange) | |
| } | |
| // Normalize inputs so cache key matches versionInRange semantics | |
| const rawVersion = change.version | |
| const rawRange = vulnEntry.range | |
| const normalizedVersion = rawVersion.trim() | |
| const normalizedRange = rawRange.trim() | |
| const cacheKey = JSON.stringify([normalizedVersion, normalizedRange]) | |
| let isInRange = rangeCheckCache.get(cacheKey) | |
| if (isInRange === undefined) { | |
| isInRange = versionInRange(normalizedVersion, normalizedRange) | |
| rangeCheckCache.set(cacheKey, isInRange) | |
| } |
There was a problem hiding this comment.
Fixed in 2728360. Normalized cache keys by trimming version and range before building key, and replaced ||| delimiter with JSON.stringify for safer keying.
|
@copilot apply changes based on the comments in this thread |
…fix TypeScript typing, normalize cache keys Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
…ate range conversion Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build cache key to avoid re-parsing same version+range combinations | ||
| const rangeCheckCache = new Map<string, boolean>() | ||
|
|
||
| // Find matching entry by ecosystem, package name (case-insensitive), and version range | ||
| let foundEntry: | ||
| | {eco: string; pkg: string; range: string; patch: string} | ||
| | undefined = undefined | ||
| for (const vulnEntry of advisoryEntries) { | ||
| if (vulnEntry.eco.toLowerCase() !== ecoLowercase) continue | ||
| if (vulnEntry.pkg.toLowerCase() !== packageLowercase) continue | ||
|
|
||
| // Normalize range once and reuse for both cache key and function call | ||
| const trimmedRangeOnce = vulnEntry.range.trim() | ||
| // Apply same normalization as versionInRange for semantic equality | ||
| const normalizedRange = trimmedRangeOnce.replace(/,\s*/g, ' ') | ||
| const cacheKey = `${normalizedChangeVersion}:${normalizedRange}` | ||
| let isInRange = rangeCheckCache.get(cacheKey) | ||
| if (isInRange === undefined) { | ||
| // 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 | ||
| isInRange = versionInRange( | ||
| normalizedChangeVersion, | ||
| normalizedRange, | ||
| {preTrimmed: true, preNormalized: true, failClosed: false} | ||
| ) | ||
| rangeCheckCache.set(cacheKey, isInRange) | ||
| } |
There was a problem hiding this comment.
The rangeCheckCache is allocated inside the per-vulnerability patch lookup and (given the current control flow) each (version, range) pair is typically evaluated only once, so the Map/key creation can add overhead without improving runtime. Consider either (1) removing the cache here, or (2) moving it to a broader scope where the same (version, range) pairs are actually re-checked (e.g., per change or per function invocation), while still keeping it bounded to avoid unbounded growth.
There was a problem hiding this comment.
Fixed in ebe10ea. Removed rangeCheckCache since each (version, range) pair is typically evaluated only once per vulnerability, eliminating unnecessary overhead.
| function versionInRange( | ||
| version: string, | ||
| range: string, | ||
| options: { | ||
| preTrimmed?: boolean | ||
| preNormalized?: boolean | ||
| failClosed?: boolean | ||
| } = {} | ||
| ): boolean { | ||
| const {preTrimmed = false, preNormalized = false, failClosed = true} = options | ||
|
|
||
| // Trim inputs if not pre-trimmed | ||
| const trimmedVersion = preTrimmed ? version : version?.trim() || '' | ||
| const trimmedRange = preTrimmed ? range : range?.trim() || '' |
There was a problem hiding this comment.
The implementation explicitly handles missing/empty inputs (and even uses optional chaining), but the function signature requires version/range to be non-null string. To align the API with actual behavior and reduce confusion for callers, consider changing the signature to accept version?: string and range?: string (or string | undefined) and then trimming without optional chaining.
There was a problem hiding this comment.
Fixed in ebe10ea. Updated function signature to accept version: string | undefined and range: string | undefined to align with the implementation that handles undefined inputs.
| // Convert GitHub API range format to semver format if not already normalized | ||
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | ||
| // Semver expects: ">=8.0.0 <=8.0.20" |
There was a problem hiding this comment.
The comment says semver expects comparator tokens without spaces (e.g., ">=8.0.0"), but the normalization implemented only replaces commas with spaces and keeps any existing whitespace after operators (e.g., ">= 8.0.0"). Either update the comment to match what the code does/what semver accepts, or adjust normalization to match the stated expectation.
| // Convert GitHub API range format to semver format if not already normalized | |
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | |
| // Semver expects: ">=8.0.0 <=8.0.20" | |
| // Convert GitHub API range format to semver-compatible format if not already normalized | |
| // GitHub uses: ">= 8.0.0, <= 8.0.20" | |
| // Semver accepts: ">= 8.0.0 <= 8.0.20" (operators may be followed by a space) |
There was a problem hiding this comment.
Fixed in ebe10ea. Updated comment to clarify semver accepts ">= 8.0.0 <= 8.0.20" (operators may be followed by a space) to match what the code actually does.
|
@copilot apply changes based on the comments in this thread + lint |
…comment Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Both version and range are valid; perform the satisfies check | ||
| return semver.satisfies(validVersion, validRange, { | ||
| includePrerelease: true | ||
| }) |
There was a problem hiding this comment.
Using includePrerelease: true unconditionally changes range semantics globally (prerelease versions may satisfy ranges that would normally exclude them). Consider making this conditional (e.g., only enable when trimmedVersion is a prerelease) or configurable via the options parameter so callers can choose the intended behavior.
There was a problem hiding this comment.
Fixed in d9bd9ca. Made includePrerelease conditional - only enabled when the version being checked is itself a prerelease (using semver.prerelease()), preserving standard range semantics for stable versions.
…range semantics Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!validVersion || !validRange) { | ||
| if (failClosed) { | ||
| core.debug( | ||
| `Invalid ${!validVersion ? 'version' : 'version range'}: version="${version}", range="${range}". Treating as vulnerable (fail closed).` | ||
| ) | ||
| } | ||
| return failClosed | ||
| } |
There was a problem hiding this comment.
The debug message only reports either an invalid version or an invalid range, but not both. If both are invalid, the log will misleadingly report only the version. Consider adjusting the message to explicitly indicate which of (version, range) is invalid (e.g., list both flags) to make troubleshooting reliable.
There was a problem hiding this comment.
Fixed in 43643c6. Debug message now reports all invalid inputs (version, range, or both) using an array that's joined with ' and ' for clear troubleshooting.
…pplicable Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Uses semver library for proper prerelease handling and range parsing | ||
| // @param version - The version to check (can be pre-trimmed) | ||
| // @param range - The version range to check against (can be pre-trimmed and/or pre-normalized) | ||
| // @param options - Configuration options | ||
| // @param options.preTrimmed - If true, assumes inputs are already trimmed (optimization) | ||
| // @param options.preNormalized - If true, assumes range is already normalized (comma-to-space conversion done) | ||
| // @param options.failClosed - If true, returns true (vulnerable) on errors; if false, returns false (no match) | ||
| function versionInRange( |
There was a problem hiding this comment.
The parameter docs are written as line comments, so TypeScript tooling won’t treat them as JSDoc. Consider converting this into a proper /** ... */ JSDoc block above versionInRange so IDE hovers and generated docs reliably include these @param details.
See below for a potential fix:
/**
* Helper to check if a version falls within a vulnerable range.
* Uses the `semver` library for proper prerelease handling and range parsing.
*
* @param version - The version to check (can be pre-trimmed).
* @param range - The version range to check against (can be pre-trimmed and/or pre-normalized).
* @param options - Configuration options.
* @param options.preTrimmed - If true, assumes inputs are already trimmed (optimization).
* @param options.preNormalized - If true, assumes range is already normalized (comma-to-space conversion done).
* @param options.failClosed - If true, returns true (vulnerable) on errors; if false, returns false (no match).
* @returns `true` if the version is considered within the vulnerable range (or on fail-closed), otherwise `false`.
*/
|
|
||
| if (!validVersion || !validRange) { | ||
| if (failClosed) { | ||
| const issues = [] |
There was a problem hiding this comment.
const issues = [] will infer any[], which weakens type safety and can trip stricter linting/TS configs. Prefer an explicit type like const issues: string[] = [] to keep this strongly typed.
| const issues = [] | |
| const issues: string[] = [] |
| // Find matching entry by ecosystem, package name (case-insensitive), and version range | ||
| let foundEntry: | ||
| | {eco: string; pkg: string; range: string; patch: string} | ||
| | undefined = undefined |
There was a problem hiding this comment.
The = undefined initializer is redundant, and the inline object type makes this harder to maintain if the advisory entry shape changes. Consider simplifying to let foundEntry: {eco: string; pkg: string; range: string; patch: string} | undefined (no initializer), or deriving the type from advisoryEntries when possible.
| | undefined = undefined | |
| | undefined |
|
@copilot apply changes based on the comments in this thread |
…initializer Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const trimmedVersion = preTrimmed ? version : version?.trim() || '' | ||
| const trimmedRange = preTrimmed ? range : range?.trim() || '' | ||
|
|
||
| if (!trimmedVersion) { | ||
| if (failClosed) { | ||
| core.debug( | ||
| `Empty or missing version for range "${range}". Treating as vulnerable (fail closed).` | ||
| ) | ||
| } | ||
| return failClosed | ||
| } | ||
| if (!trimmedRange) { | ||
| if (failClosed) { | ||
| core.debug( | ||
| `Empty or missing version range for version "${version}". Treating as vulnerable (fail closed).` | ||
| ) | ||
| } | ||
| return failClosed | ||
| } |
There was a problem hiding this comment.
These debug messages interpolate the raw version/range arguments, which can be misleading for whitespace-only inputs (the trim happens before the emptiness check). Consider logging the trimmed/normalized values (or JSON.stringify(...)) so it’s clear why the function treated inputs as empty.
|
@copilot apply changes based on the comments in this thread |
|
@copilot apply changes based on the comments in [this thread](#7 (review) |
|
Hello? |
Purpose
Addresses issues identified in PR actions#1045 review and subsequent feedback.
Changes Made
1. Semver/range parsing with configurable fail behavior
Replaced custom semver parsing (which incorrectly handled prereleases like
107.0.0-preview.1→parseInt('0-preview', 10)=0and silently ignored unparseable conditions) with the standardsemverlibrary:Explicit validation for reliable fail-closed semantics:
semver.valid()andsemver.validRange()for explicit validationsemver.satisfies()returnsfalsefor invalid inputs without throwing exceptionsCorrect prerelease handling:
includePrereleaseconditionally based on whether the version being checked is a prereleaseincludePrereleasewhensemver.prerelease(version) !== nullConfigurable fail behavior:
optionsparameter withfailClosedflag (defaults totruefor security)true= vulnerable) on parse errors or empty inputsfalse= no match) to avoid incorrect patch recommendations on invalid rangesversion: string | undefined, range: string | undefinedto match implementationPerformance optimizations:
preTrimmedoption to skip redundant trimming in hot loopspreNormalizedoption to skip redundant comma-to-space conversion in hot loops{preTrimmed: true, preNormalized: true, failClosed: false}Input sanitization:
const trimmed = input?.trim() || ''pattern for safe trimming2. Rows array scoping
Fixed bug where
rowsdeclared at function level accumulated across manifest iterations. Moved declaration inside manifest loop to scope per-iteration, preventing duplicate/incorrect tables when multiple manifests exist.3. Case-insensitive package and ecosystem matching
NuGet and similar ecosystems are case-insensitive. Normalize both sides to lowercase:
entry.pkg.toLowerCase() === packageLowercasevulnEntry.eco.toLowerCase() === ecoLowercase4. Error logging
Extract message from Error objects instead of
[object Object]:5. Removed redundant TypeScript types
Removed
@types/semverfrom devDependencies as semver v7+ includes built-in TypeScript type definitions, preventing potential type conflicts.6. Performance optimization
Optimized version range checking for patch selection:
change.versiononce outside the loop (trim only)vulnEntry.rangeonce per entry (trim + comma-to-space conversion)versionInRange()to skip redundant operationsundefinedinstead ofnullfor better TypeScript type inferenceRelated Issues
Implements review comments from actions#1045 (review)
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.