Add list of resolved vulnerabilities to the PR Comment / Logs#1032
Add list of resolved vulnerabilities to the PR Comment / Logs#1032david-wiggs wants to merge 8 commits intoactions:mainfrom
Conversation
- Add resolved vulnerabilities detection and display - Show positive feedback when vulnerabilities are resolved by removing/upgrading packages - Add resolved-vulnerabilities output for workflow automation - Include resolved vulnerabilities in PR comments and job summaries - Add comprehensive tests and documentation - Addresses GitHub issue actions#717 Co-authored-by: GitHub Copilot <copilot@github.com>
- Remove markdown bold formatting from summary counts (use plain text) - Use HTML <strong> tags instead of markdown ** for better rendering - Add celebration emoji to resolved vulnerabilities count - Clean up heading formatting for resolved vulnerabilities section - Ensure consistent formatting across summary and detailed sections
…re table, remove duplicate text - Create separate summaryListHtml and summaryListMarkdown arrays to fix ** rendering - Add newline after resolved vulnerabilities header for better spacing - Remove duplicate resolved vulnerabilities text from main summary - Ensures proper bold formatting in both GitHub Action summaries and PR comments
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to track and display resolved vulnerabilities when vulnerable dependencies are removed from a project. The feature provides positive feedback to developers about the security improvements their changes introduce, without requiring any API changes. Resolved vulnerabilities are identified by examining removed dependencies that had known vulnerabilities.
Key changes:
- New
resolved-vulnerabilitiesoutput containing JSON data about resolved vulnerabilities - Visual feedback in PR summaries and logs highlighting resolved vulnerabilities with positive messaging
- Support for both HTML (Action summaries) and Markdown (PR comments) formatting
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/schemas.ts | Adds ResolvedVulnerability and ResolvedVulnerabilities schemas with fields for tracking resolved vulnerability details |
| src/resolved-vulnerabilities.ts | New module implementing getResolvedVulnerabilities() to extract vulnerabilities from removed dependencies |
| src/summary.ts | Adds functions to display resolved vulnerabilities in summaries, with separate HTML/Markdown formatting; updates existing summary functions to include resolved vulnerabilities |
| src/main.ts | Integrates resolved vulnerabilities feature by computing them from changes and displaying them when vulnerability_check is enabled |
| tests/resolved-vulnerabilities.test.ts | Comprehensive test suite for the getResolvedVulnerabilities() function covering various scenarios |
| tests/summary.test.ts | Updates all existing tests to pass the new emptyResolvedVulnerabilities parameter |
| scripts/create_summary.ts | Updates summary creation script to include the new resolved vulnerabilities parameter |
| docs/examples.md | Adds documentation and example workflow for accessing the resolved-vulnerabilities output |
| README.md | Documents the new resolved-vulnerabilities output in the action outputs section |
| dist/index.js | Compiled distribution file reflecting all source code changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const summaryListHtml: string[] = [ | ||
| // Add resolved vulnerabilities as positive feedback first | ||
| ...(resolvedVulnerabilities.length > 0 | ||
| ? [ | ||
| `${icons.check} <strong>${resolvedVulnerabilities.length}</strong> vulnerability(ies) resolved 🎉` | ||
| ] | ||
| : []), |
There was a problem hiding this comment.
Resolved vulnerabilities are shown in the summary list even when config.vulnerability_check is disabled. This is inconsistent with the treatment of other vulnerability information. Consider wrapping this section with a check for config.vulnerability_check:
...(config.vulnerability_check && resolvedVulnerabilities.length > 0
? [
`${icons.check} <strong>${resolvedVulnerabilities.length}</strong> vulnerability(ies) resolved 🎉`
]
: []),| const summaryListMarkdown: string[] = [ | ||
| // Add resolved vulnerabilities as positive feedback first | ||
| ...(resolvedVulnerabilities.length > 0 | ||
| ? [ | ||
| `${icons.check} **${resolvedVulnerabilities.length}** vulnerability(ies) resolved 🎉` | ||
| ] | ||
| : []), |
There was a problem hiding this comment.
Resolved vulnerabilities are shown in the markdown summary list even when config.vulnerability_check is disabled. This is inconsistent with the treatment of other vulnerability information. Consider wrapping this section with a check for config.vulnerability_check:
...(config.vulnerability_check && resolvedVulnerabilities.length > 0
? [
`${icons.check} **${resolvedVulnerabilities.length}** vulnerability(ies) resolved 🎉`
]
: []),| The `resolved-vulnerabilities` output is a JSON array containing information about vulnerabilities that were resolved by removing or upgrading packages. Each resolved vulnerability includes: | ||
| - `severity`: The severity level of the resolved vulnerability | ||
| - `advisory_ghsa_id`: The GitHub Advisory Database ID | ||
| - `advisory_summary`: A summary of the vulnerability | ||
| - `advisory_url`: URL to the advisory | ||
| - `package_name`: Name of the package that had the vulnerability | ||
| - `package_version`: Version of the package that had the vulnerability |
There was a problem hiding this comment.
The documentation states that resolved vulnerabilities are found "by removing or upgrading packages," but the implementation doesn't distinguish between these two scenarios. When a vulnerable package is completely removed (not upgraded to a safer version), treating this as a "resolved vulnerability" may be misleading. The vulnerability isn't technically resolved—the functionality may have been removed entirely or moved elsewhere.
Consider either:
- Updating the documentation to be more precise about what "resolved" means in this context
- Enhancing the logic to differentiate between packages that were upgraded (removed old version + added new version) vs. completely removed
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@david-wiggs thanks for this PR, this seems like useful functionality and i understand from the community comments on #717 that it's a desirable feature. a couple of requests:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
src/main.ts:148
resolvedVulnerabilitiesis computed from the fullchangeslist before scope/allow-list filtering. This can surface “resolved vulnerabilities” for scopes that the action is configured to ignore viafail-on-scopes(and for advisories that may be allowed viaallow-ghsas). To keep the output consistent with what the action evaluates, consider deriving resolved vulnerabilities fromscopedChanges(and potentially after allow-list filtering, depending on intended semantics).
// Extract resolved vulnerabilities from removed dependencies
const resolvedVulnerabilities = getResolvedVulnerabilities(changes)
const scopedChanges = filterChangesByScopes(config.fail_on_scopes, changes)
const filteredChanges = filterAllowedAdvisories(
config.allow_ghsas,
scopedChanges
)
src/summary.ts:123
- Same as above: the markdown status list includes the resolved-vulnerabilities line without checking
config.vulnerability_check, which can surface vulnerability-related messaging when vulnerability checks are disabled.
const summaryListMarkdown: string[] = [
// Add resolved vulnerabilities as positive feedback first
...(resolvedVulnerabilities.length > 0
? [
`${icons.check} **${resolvedVulnerabilities.length}** vulnerability(ies) resolved 🎉`
]
: []),
...(config.vulnerability_check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (config.vulnerability_check && resolvedVulnerabilities.length > 0) { | ||
| core.setOutput('resolved-vulnerabilities', JSON.stringify(resolvedVulnerabilities)) | ||
| summary.addResolvedVulnerabilitiesToSummary(resolvedVulnerabilities) | ||
| printResolvedVulnerabilitiesBlock(resolvedVulnerabilities) |
There was a problem hiding this comment.
The resolved-vulnerabilities output is only set when the list is non-empty. This makes downstream workflow conditions like steps.review.outputs.resolved-vulnerabilities != '[]' unreliable (unset outputs become empty strings). Consider always setting the output when config.vulnerability_check is enabled (use '[]' when empty) for consistency with vulnerable-changes and to simplify consumer logic.
| if (config.vulnerability_check && resolvedVulnerabilities.length > 0) { | |
| core.setOutput('resolved-vulnerabilities', JSON.stringify(resolvedVulnerabilities)) | |
| summary.addResolvedVulnerabilitiesToSummary(resolvedVulnerabilities) | |
| printResolvedVulnerabilitiesBlock(resolvedVulnerabilities) | |
| if (config.vulnerability_check) { | |
| core.setOutput( | |
| 'resolved-vulnerabilities', | |
| JSON.stringify(resolvedVulnerabilities) | |
| ) | |
| if (resolvedVulnerabilities.length > 0) { | |
| summary.addResolvedVulnerabilitiesToSummary(resolvedVulnerabilities) | |
| printResolvedVulnerabilitiesBlock(resolvedVulnerabilities) | |
| } |
| const summaryListHtml: string[] = [ | ||
| // Add resolved vulnerabilities as positive feedback first | ||
| ...(resolvedVulnerabilities.length > 0 | ||
| ? [ | ||
| `${icons.check} <strong>${resolvedVulnerabilities.length}</strong> vulnerability(ies) resolved 🎉` | ||
| ] | ||
| : []), | ||
| ...(config.vulnerability_check |
There was a problem hiding this comment.
The “resolved vulnerabilities” status line is added even when config.vulnerability_check is false (it only checks resolvedVulnerabilities.length). This can cause the summary to mention vulnerabilities despite the user disabling vulnerability checks. Consider gating these list entries on config.vulnerability_check as well.
This issue also appears on line 116 of the same file.
| export function addResolvedVulnerabilitiesToSummary( | ||
| resolvedVulnerabilities: ResolvedVulnerabilities | ||
| ): void { | ||
| if (resolvedVulnerabilities.length === 0) { | ||
| return | ||
| } | ||
|
|
||
| core.summary.addHeading('Resolved Vulnerabilities', 2) | ||
| core.summary.addRaw(`${icons.check} Great job! This PR resolves <strong>${resolvedVulnerabilities.length}</strong> ${resolvedVulnerabilities.length === 1 ? 'vulnerability' : 'vulnerabilities'}:`) | ||
| core.summary.addRaw('') | ||
|
|
||
| const tableRows: SummaryTableRow[] = [ | ||
| [ | ||
| {data: 'Package', header: true}, | ||
| {data: 'Version', header: true}, | ||
| {data: 'Vulnerability', header: true}, | ||
| {data: 'Severity', header: true} | ||
| ] | ||
| ] | ||
|
|
||
| for (const vuln of resolvedVulnerabilities) { | ||
| tableRows.push([ | ||
| `${vuln.manifest} » <strong>${vuln.package_name}</strong>`, | ||
| vuln.package_version, | ||
| renderUrl(vuln.advisory_url, vuln.advisory_summary), | ||
| vuln.severity | ||
| ]) | ||
| } | ||
|
|
||
| core.summary.addTable(tableRows) | ||
| core.summary.addRaw('Keep up the great work securing your dependencies! 🎉') | ||
| } |
There was a problem hiding this comment.
New resolved-vulnerabilities summary rendering is not covered by tests. There are existing Jest tests for summary formatting in this repo; adding at least one test that asserts the heading/table (and pluralization) for a non-empty resolvedVulnerabilities list would help prevent regressions.
| - name: 'Celebrate Resolved Vulnerabilities' | ||
| if: steps.review.outputs.resolved-vulnerabilities != '[]' | ||
| env: | ||
| RESOLVED_VULNERABILITIES: ${{ steps.review.outputs.resolved-vulnerabilities }} | ||
| run: | | ||
| echo "🎉 Great job! This PR resolves vulnerabilities:" | ||
| echo "$RESOLVED_VULNERABILITIES" | jq -r '.[] | "- \(.severity | ascii_upcase): \(.advisory_summary) in \(.package_name)@\(.package_version)"' |
There was a problem hiding this comment.
The workflow example checks steps.review.outputs.resolved-vulnerabilities != '[]', but the action currently does not set this output at all when there are zero resolved vulnerabilities. That makes the condition true for the empty-string case and will cause jq to fail. Either adjust the example to handle unset/empty outputs, or (preferably) ensure the action always sets the output to '[]' when enabled.
| snapshot_warnings: z.string(), | ||
| resolved_vulnerabilities: ResolvedVulnerabilitiesSchema.optional().default([]) |
There was a problem hiding this comment.
ComparisonResponseSchema now includes a resolved_vulnerabilities field, but the implementation derives resolved vulnerabilities locally from changes and doesn’t read this property. If the API response does not actually include resolved_vulnerabilities, this schema can be misleading; if it does, the action should prefer using it to avoid the false-positive cases around upgrades. Consider either removing this field from the schema or wiring it through and using it as the source of truth.
| snapshot_warnings: z.string(), | |
| resolved_vulnerabilities: ResolvedVulnerabilitiesSchema.optional().default([]) | |
| snapshot_warnings: z.string() |
| // Filter for removed dependencies that have vulnerabilities | ||
| const removedChangesWithVulns = changes.filter( | ||
| change => change.change_type === 'removed' && | ||
| change.vulnerabilities && | ||
| change.vulnerabilities.length > 0 | ||
| ) |
There was a problem hiding this comment.
getResolvedVulnerabilities() treats any vulnerability on a change_type: 'removed' package as “resolved”. This can produce false positives during upgrades (the old version is removed, but the same GHSA may still exist on the added version or elsewhere in the diff). Consider filtering out advisories that still appear in non-removed changes (e.g., match by ecosystem/manifest/package_name/advisory_ghsa_id), or rename the concept/output to “vulnerabilities on removed dependencies” to avoid overstating remediation.
| - `vulnerable-changes` holds information about dependency changes with vulnerable dependencies in a JSON format. | ||
| - `invalid-license-changes` holds information about invalid or non-compliant license dependency changes in a JSON format. | ||
| - `denied-changes` holds information about denied dependency changes in a JSON format. | ||
| - `resolved-vulnerabilities` holds information about vulnerabilities that have been resolved by removing or upgrading packages in a JSON format. |
There was a problem hiding this comment.
README documents a new resolved-vulnerabilities action output, but action.yml does not declare it under outputs. For GitHub Actions consumers and Marketplace docs, outputs should be listed in action.yml as well.
| - `resolved-vulnerabilities` holds information about vulnerabilities that have been resolved by removing or upgrading packages in a JSON format. |
This is a first attempt at #717 without any changes to the API